diff --git a/src/common/constants/memory.ts b/src/common/constants/memory.ts index bf650683d6..35b1ad3f9f 100644 --- a/src/common/constants/memory.ts +++ b/src/common/constants/memory.ts @@ -9,7 +9,8 @@ * per-project notes; never committed to the repo, survives * workspaces; carried by the settings backup only when the * user opts into the project bundle) - * - workspace -> /memory/ (host-local, deleted with the workspace) + * - workspace -> /memory/ of the task-tree OWNER (host-local, deleted + * with that workspace; sub-agents share their parent's store) */ /** Virtual root prefix all memory paths are expressed under. */ diff --git a/src/common/utils/tools/toolDefinitions.ts b/src/common/utils/tools/toolDefinitions.ts index c30c56e224..16c2d7b07f 100644 --- a/src/common/utils/tools/toolDefinitions.ts +++ b/src/common/utils/tools/toolDefinitions.ts @@ -2511,7 +2511,7 @@ export const TOOL_DEFINITIONS = { "Scopes (all paths are virtual):\n" + "- /memories/global/... — personal, permanent, shared across all projects\n" + "- /memories/project/... — private notes about this project; host-local, never committed to the repo (included in the settings backup only when the user opts in), survives workspaces\n" + - "- /memories/workspace/... — scratch state for this workspace; deleted with the workspace\n" + + "- /memories/workspace/... — scratch state for this workspace, shared with its sub-agents (a sub-agent reads and writes its parent's workspace notes); deleted with the owning workspace\n" + "Commands:\n" + "- view: list a directory (up to 2 levels, dotfiles excluded) or show a file with line numbers (offset/limit supported)\n" + "- create: create a new file; ERRORS if the file already exists (to overwrite: delete first, then create)\n" + diff --git a/src/node/config/index.ts b/src/node/config/index.ts index 4ecc7e3419..4b1b9b192c 100644 --- a/src/node/config/index.ts +++ b/src/node/config/index.ts @@ -884,6 +884,11 @@ interface ConfigLoadFailureState { // re-log the same corrupt-config error once per instance. const configLoadFailureStates = new Map(); +/** Location of config.json under a Xum root (also used by callers that must stat it without a Config). */ +export function configFilePath(rootDir: string): string { + return path.join(rootDir, "config.json"); +} + function configLoadFailureState(configFile: string): ConfigLoadFailureState { let state = configLoadFailureStates.get(configFile); if (!state) { @@ -934,6 +939,21 @@ export class Config { readonly sessionsDir: string; readonly srcDir: string; private readonly configFile: string; + /** + * Cheap durable change signal for config.json: one stat, no parse. Any + * backend's rewrite changes size/mtime (atomic replace also changes the + * inode), so a memo built against a previous stamp knows to rebuild — + * unlike onConfigChanged, which only fires for THIS process's edits. A + * missing/unreadable file yields a distinct stamp. + */ + configFileStamp(): string { + try { + const st = fs.statSync(this.configFile, { bigint: true }); + return `${st.dev}:${st.ino}:${st.size}:${st.mtimeNs}`; + } catch { + return "missing"; + } + } private readonly providersConfigStore: ProvidersConfigStore; private readonly emitter = new EventEmitter(); /** @@ -969,7 +989,7 @@ export class Config { this.rootDir = sessionLocator.rootDir; this.sessionsDir = sessionLocator.sessionsDir; this.srcDir = sessionLocator.srcDir; - this.configFile = path.join(this.rootDir, "config.json"); + this.configFile = configFilePath(this.rootDir); this.providersConfigStore = providersConfigStore ?? new ProvidersConfigStore(this.rootDir); } diff --git a/src/node/orpc/routerSubscriptions.test.ts b/src/node/orpc/routerSubscriptions.test.ts index 2e45423285..8eb45a00f7 100644 --- a/src/node/orpc/routerSubscriptions.test.ts +++ b/src/node/orpc/routerSubscriptions.test.ts @@ -13,7 +13,11 @@ import { TestClock } from "effect/testing"; import { SUBSCRIPTION_HEARTBEAT_INTERVAL_MS } from "@/common/utils/withQueueHeartbeat"; import { disposeAppRuntime, makeAppRuntime } from "@/node/services/di/appRuntime"; import type { ORPCContext } from "./context"; -import { subscribeWorkspaceActivity, subscribeDesignExperiment } from "./routerSubscriptions"; +import { + subscribeWorkspaceActivity, + subscribeDesignExperiment, + subscribeMemoryChanges, +} from "./routerSubscriptions"; test("subscription handlers forward the oRPC runtime Clock", async () => { const app = makeAppRuntime(TestClock.layer()); @@ -37,6 +41,71 @@ test("subscription handlers forward the oRPC runtime Clock", async () => { expect(workspaceService.listenerCount("activity")).toBe(0); }); +test("memory subscriptions match workspace-scope events on the shared memory owner", async () => { + // Workspace-scope change events carry the memory OWNER (task-tree root); + // a sub-agent's subscription must see edits to the notebook it shares. + const app = makeAppRuntime(TestClock.layer()); + const memoryService = new EventEmitter(); + const memoryConsolidationService = new EventEmitter(); + const controller = new AbortController(); + const ownerOf = new Map([["ws-child", "ws-owner"]]); + const context = { + "effect/context": app.context, + workspaceService: { getInfo: () => Promise.resolve(null) }, + memoryService: Object.assign(memoryService, { + resolveWorkspaceMemoryOwnerId: (workspaceId: string) => + ownerOf.get(workspaceId) ?? workspaceId, + }), + memoryConsolidationService, + } as unknown as ORPCContext; + const stream = subscribeMemoryChanges(context, "ws-child", controller.signal); + try { + const first = stream.next(); + // The listener attaches once the generator has started running. + while (memoryService.listenerCount("change") === 0) { + await new Promise((resolve) => setTimeout(resolve, 0)); + } + const base = { + scope: "workspace", + path: "/memories/workspace/n.md", + actor: "agent", + projectPath: "", + }; + memoryService.emit("change", { ...base, workspaceId: "ws-other" }); + memoryService.emit("change", { ...base, workspaceId: "ws-child" }); + memoryService.emit("change", { ...base, workspaceId: "ws-owner" }); + // Global scope is never filtered: it marks the end of the batch, so + // receiving it second proves the other/child events were dropped. + const marker = { ...base, scope: "global", path: "/memories/global/g.md", workspaceId: "" }; + memoryService.emit("change", marker); + expect((await first).value).toEqual({ ...base, workspaceId: "ws-owner" }); + expect((await stream.next()).value).toEqual(marker); + + // Ownership change for THIS workspace (owner removed): synthesized + // root-addressed refresh + status refresh, now addressed to the new owner. + ownerOf.set("ws-child", "ws-child"); + memoryService.emit("ownersInvalidated", ["ws-unrelated"]); + memoryService.emit("ownersInvalidated", ["ws-child"]); + expect((await stream.next()).value).toEqual({ + scope: "workspace", + path: "/memories/workspace", + actor: "user", + workspaceId: "ws-child", + projectPath: "", + }); + expect((await stream.next()).value).toEqual({ + kind: "consolidation_status", + workspaceId: "ws-child", + projectPath: "", + }); + } finally { + controller.abort(); + await stream.return(undefined); + await disposeAppRuntime(app.managed); + } + expect(memoryService.listenerCount("change")).toBe(0); +}); + test("Design subscriptions publish sibling changes only after client shutdown", async () => { using temp = new DisposableTempDir("design-subscription"); const flags = path.join(temp.path, EXPERIMENT_OVERRIDES_FILE_NAME); diff --git a/src/node/orpc/routerSubscriptions.ts b/src/node/orpc/routerSubscriptions.ts index 8c0a374f89..537c7ae831 100644 --- a/src/node/orpc/routerSubscriptions.ts +++ b/src/node/orpc/routerSubscriptions.ts @@ -24,6 +24,7 @@ import type { LogEntry } from "@/node/services/logBuffer"; import { subscribeLogFeed } from "@/node/services/logBuffer"; import { resolveMemoryProjectIdentity, + toVirtualPath, type MemoryChangeEvent, } from "@/node/services/memoryService"; @@ -207,20 +208,48 @@ export function subscribeMemoryChanges( validate?.(); const metadata = workspaceId ? await context.workspaceService.getInfo(workspaceId) : null; const projectPath = metadata ? resolveMemoryProjectIdentity(metadata) : null; + // Workspace-scope events carry the memory OWNER (task-tree root), which is + // also the store this subscriber's workspace displays. Resolved per event + // (memoized, cheap): the owner can change while the tab stays open — a + // removed owner makes the child fall back to its own store. yield* runtimeSubscription(context, { signal, subscribe: (emit) => { const onChange = (event: MemoryChangeEvent) => { - if (event.scope === "workspace" && event.workspaceId !== workspaceId) return; + if ( + event.scope === "workspace" && + event.workspaceId !== + (workspaceId + ? context.memoryService.resolveWorkspaceMemoryOwnerId(workspaceId) + : null) + ) + return; if (event.scope === "project" && event.projectPath !== projectPath) return; emit.push(event); }; const onStatusChange = (event: MemoryConsolidationStatusChangeEventPayload) => emit.push(event); + // Ownership changed for this workspace (its owner was removed; it + // now reads its own store). No mutation event accompanies a removal, + // so synthesize a root-addressed refresh for the file list and the + // consolidation status, both of which described the old store. + const onOwnersInvalidated = (workspaceIds: string[]) => { + if (!workspaceId || !workspaceIds.includes(workspaceId)) return; + emit.push({ + scope: "workspace", + path: toVirtualPath("workspace", ""), + actor: "user", + workspaceId: context.memoryService.resolveWorkspaceMemoryOwnerId(workspaceId), + projectPath: projectPath ?? "", + }); + emit.push({ kind: "consolidation_status", workspaceId, projectPath: projectPath ?? "" }); + }; context.memoryService.on("change", onChange); + context.memoryService.on("ownersInvalidated", onOwnersInvalidated); context.memoryConsolidationService.on("statusChange", onStatusChange); return () => { context.memoryService.off("change", onChange); + context.memoryService.off("ownersInvalidated", onOwnersInvalidated); context.memoryConsolidationService.off("statusChange", onStatusChange); }; }, diff --git a/src/node/services/agentSession.memoryContext.test.ts b/src/node/services/agentSession.memoryContext.test.ts index 0e9d207763..e349b4c845 100644 --- a/src/node/services/agentSession.memoryContext.test.ts +++ b/src/node/services/agentSession.memoryContext.test.ts @@ -87,7 +87,8 @@ function createSession(args: { interface PrivateSessionAccess { resolveMemoryContext: ( modelString: string, - options?: Parameters[2] + options?: Parameters[2], + cache?: Map ) => Promise; getPostCompactionAttachmentsIfNeeded: () => Promise; } @@ -127,6 +128,52 @@ describe("AgentSession memory context", () => { expect(await priv.resolveMemoryContext("test-model")).toEqual(context); expect(await priv.resolveMemoryContext("test-model")).toEqual(context); expect(buildMemorySessionContext).toHaveBeenCalledTimes(1); + + // A write by another task-tree member to the shared workspace notebook + // invalidates from outside; the next resolve rebuilds from disk. + session.invalidateMemoryContext(); + expect(await priv.resolveMemoryContext("test-model")).toEqual(context); + expect(buildMemorySessionContext).toHaveBeenCalledTimes(2); + } finally { + await session.dispose(); + } + }); + + test("does not cache a context whose build overlapped an invalidation", async () => { + using sessionDir = new DisposableTempDir("agent-session-memory-context-race"); + const { historyService, cleanup } = await createTestHistoryService(); + historyCleanup = cleanup; + + let release!: () => void; + const gate = new Promise((resolve) => (release = resolve)); + const stale: MemorySessionContext = { indexEntries: [], hotMemoriesBlock: "stale" }; + const fresh: MemorySessionContext = { indexEntries: [], hotMemoriesBlock: "fresh" }; + let calls = 0; + const buildMemorySessionContext = mock(async () => { + calls++; + if (calls === 1) await gate; + return calls === 1 ? stale : fresh; + }); + const session = createSession({ + historyService, + sessionDir: path.join(sessionDir.path, WORKSPACE_ID), + buildMemorySessionContext, + }); + const priv = session as unknown as PrivateSessionAccess; + + try { + // A rollover candidate builds into its own staged map. + const staged = new Map(); + const building = priv.resolveMemoryContext("test-model", undefined, staged); + // A sibling session writes the shared notebook mid-build: the files + // the build read are already stale. + session.invalidateMemoryContext(); + release(); + expect(await building).toEqual(stale); + // Served once for the request that needed it, but never cached. + expect(staged.size).toBe(0); + expect(await priv.resolveMemoryContext("test-model")).toEqual(fresh); + expect(buildMemorySessionContext).toHaveBeenCalledTimes(2); } finally { await session.dispose(); } diff --git a/src/node/services/agentSession.ts b/src/node/services/agentSession.ts index e447b5ca4e..891de54f3c 100644 --- a/src/node/services/agentSession.ts +++ b/src/node/services/agentSession.ts @@ -1049,6 +1049,24 @@ export class AgentSession { * prompt-cache-stable bytes without preserving stale files forever. */ private memoryContextByModelString = new Map(); + + /** + * Drop the cached memory context so the next stream rebuilds the index and + * hot set from disk. Own memory tool calls clear it on tool-call-end; this + * entry point is for writes by OTHER sessions to a store this session also + * reads (a sub-agent editing the task tree's shared workspace notes). + */ + invalidateMemoryContext(): void { + this.memoryContextByModelString.clear(); + // A build already awaiting buildMemorySessionContext read the pre-write + // files, and a rollover candidate stages its cache in a separate map that + // is installed later: bumping the generation stops both from + // (re)populating the cache with the stale snapshot. + this.memoryContextGeneration++; + } + + private memoryContextGeneration = 0; + /** * Cache the last-known experiment state so we don't spam metadata refresh * when post-compaction context is disabled. @@ -5490,6 +5508,7 @@ export class AgentSession { message: "Full request preparation is unavailable; use /compact or restart.", }); const cache = new Map(); + const cacheGeneration = this.memoryContextGeneration; // Admission must not pause the goal yet, but the pinned tools must match the later manual pause. let prospectiveGoalStatusForToolAvailability: StreamMessageOptions["prospectiveGoalStatusForToolAvailability"]; if (manualIntervention && this.workspaceGoalService) { @@ -5593,7 +5612,14 @@ export class AgentSession { : prepared; return Ok({ start: (startOptions) => { - this.memoryContextByModelString = cache; + // A shared-notebook write by another tree session while this + // candidate was prepared invalidated the installed map only; the + // staged one is then unavoidably stale for this request and must + // not be reused by later turns. + this.memoryContextByModelString = + cacheGeneration === this.memoryContextGeneration + ? cache + : new Map(); return prepared.data.start(startOptions); }, [Symbol.asyncDispose]: () => prepared.data[Symbol.asyncDispose](), @@ -10367,6 +10393,7 @@ export class AgentSession { return cached.context ?? undefined; } + const generation = this.memoryContextGeneration; // Guard for test mocks that may not implement buildMemorySessionContext. const context = typeof this.aiService.buildMemorySessionContext === "function" @@ -10375,13 +10402,16 @@ export class AgentSession { tokenBudgetActive, }) : null; - cache.set(modelString, { - context, - includesHotMemories: includeHotMemories, - tokenBudgetActive, - memoryEnabled, - hotSetEnabled, - }); + // Invalidated mid-build: serve this snapshot once, do not cache it. + if (generation === this.memoryContextGeneration) { + cache.set(modelString, { + context, + includesHotMemories: includeHotMemories, + tokenBudgetActive, + memoryEnabled, + hotSetEnabled, + }); + } return context ?? undefined; } diff --git a/src/node/services/di/layers/core.ts b/src/node/services/di/layers/core.ts index 6abe54f1de..225ff77f86 100644 --- a/src/node/services/di/layers/core.ts +++ b/src/node/services/di/layers/core.ts @@ -57,7 +57,7 @@ import { MCPConfigService } from "@/node/services/mcpConfigService"; import { MCPServerManager } from "@/node/services/mcpServerManager"; import { MemoryConsolidationService } from "@/node/services/memoryConsolidationService"; import { MemoryMetaService } from "@/node/services/memoryMeta"; -import { MemoryService } from "@/node/services/memoryService"; +import { MemoryService, type MemoryChangeEvent } from "@/node/services/memoryService"; import { ProviderService } from "@/node/services/providerService"; import { SessionUsageService } from "@/node/services/sessionUsageService"; import { StreamManager } from "@/node/services/streamManager"; @@ -573,6 +573,25 @@ export const CoreWiringLive: Layer.Layer< workspaceService.emitWorkflowRunActivity(event); turnRequestBuilderBindings.workflowResultContinuationSender = workspaceService; workspaceService.setMemoryConsolidationService(memoryConsolidationService); + // Workspace-scope change events carry the memory OWNER (task-tree root); + // every live session resolving to that owner reads the same notebook. + memoryService.on("change", (event: MemoryChangeEvent) => { + if (event.scope !== "workspace" || event.workspaceId === "") return; + // One config snapshot for the whole pass: cold owner lookups would + // otherwise parse the config once per live session, synchronously. + let cfg: ReturnType | undefined; + const snapshot = () => (cfg ??= config.loadConfigOrDefault()); + workspaceService.invalidateMemoryContextWhere( + (workspaceId) => + memoryService.resolveWorkspaceMemoryOwnerId(workspaceId, snapshot) === event.workspaceId + ); + }); + // Ownership itself changed (an owner was removed while its sub-agents + // live on): those sessions' cached contexts still describe the old store. + memoryService.on("ownersInvalidated", (workspaceIds: string[]) => { + const affected = new Set(workspaceIds); + workspaceService.invalidateMemoryContextWhere((workspaceId) => affected.has(workspaceId)); + }); if (opts.devToolsService) { // DevTools debug-log cleanup when workspaces are archived/removed. workspaceService.setDevToolsService(opts.devToolsService); diff --git a/src/node/services/memoryConsolidation.test.ts b/src/node/services/memoryConsolidation.test.ts index 2acadf5230..9f1a60d364 100644 --- a/src/node/services/memoryConsolidation.test.ts +++ b/src/node/services/memoryConsolidation.test.ts @@ -313,6 +313,48 @@ describe("consolidation memory tool rails", () => { ).toContain("polished"); }); + it("enforces pin protection inside the mutation against the store the command binds to", async () => { + using fixture = await createFixture(); + const sessionMemory = path.join(fixture.xumHome, "sessions", fixture.ctx.workspaceId, "memory"); + await fsPromises.mkdir(sessionMemory, { recursive: true }); + await fsPromises.writeFile(path.join(sessionMemory, "pinned.md"), "keep me\n"); + await fixture.metaService.setPinned( + memoryLogicalKey("workspace", "pinned.md", { + projectPath: fixture.ctx.projectPath, + workspaceId: fixture.ctx.workspaceId, + }), + true + ); + // The tool's pre-check resolves the shared-store owner on its own; make + // that resolution disagree with the command's (as a transiently unreadable + // config.json can, falling back to a different store) so the pre-check + // looks at the wrong sidecar key and passes. The command itself must + // still refuse: its check runs in-lock against the owner its store is + // bound to. + const resolve = spyOn(fixture.memoryService, "resolveWorkspaceMemoryOwnerId"); + resolve.mockImplementationOnce(() => "ws-elsewhere"); + try { + const deletion = await execute(fixture.tool, { + command: "delete", + path: "/memories/workspace/pinned.md", + }); + expect(deletion.success).toBe(false); + if (!deletion.success) expect(deletion.error).toContain("pinned"); + expect(await pathExists(path.join(sessionMemory, "pinned.md"))).toBe(true); + resolve.mockImplementationOnce(() => "ws-elsewhere"); + const rename = await execute(fixture.tool, { + command: "rename", + old_path: "/memories/workspace/pinned.md", + new_path: "/memories/workspace/moved.md", + }); + expect(rename.success).toBe(false); + if (!rename.success) expect(rename.error).toContain("pinned"); + expect(await pathExists(path.join(sessionMemory, "pinned.md"))).toBe(true); + } finally { + resolve.mockRestore(); + } + }); + it("rejects deleting or renaming a directory that contains a pinned file", async () => { using fixture = await createFixture(); const nestedDir = path.join(fixture.globalMemoryDir, "nested"); diff --git a/src/node/services/memoryConsolidation.ts b/src/node/services/memoryConsolidation.ts index 509da74270..cf84b90498 100644 --- a/src/node/services/memoryConsolidation.ts +++ b/src/node/services/memoryConsolidation.ts @@ -244,6 +244,11 @@ export function createConsolidationMemoryTool(args: { // Deletes/renames may target a directory (MemoryService removes // recursively), so reject when the path itself OR anything under it is // pinned — otherwise `delete dir/` would silently destroy dir/pinned.md. + // This pre-check gives the model (and dry-run staging) early feedback; + // the AUTHORITATIVE check runs inside MemoryService's mutation lock + // against the owner the command's store is actually bound to + // (`rejectPinned` below): the owner resolved here can differ from the + // command's when config.json is transiently unreadable in between. if (target.command === "delete" || target.command === "rename") { const { scope, relPath } = parseMemoryPath(target.path); assert( @@ -253,7 +258,11 @@ export function createConsolidationMemoryTool(args: { const entries = await metaService.getEntries(); const key = memoryLogicalKey(scope, relPath, { projectPath: ctx.projectPath, - workspaceId: ctx.workspaceId, + // Sidecar keys follow the shared store's owner (MemoryService.logicalKeyFor). + workspaceId: + ctx.workspaceId === "" + ? "" + : memoryService.resolveWorkspaceMemoryOwnerId(ctx.workspaceId), }); const subtreePrefix = `${key}/`; for (const [entryKey, entry] of entries) { @@ -327,6 +336,7 @@ export function createConsolidationMemoryTool(args: { const result = await executeMemoryCommand(memoryService, ctx, input, () => null, toolCallId, { expectedTargetFingerprint: args.expectedTargetFingerprints?.get(toolCallId), abortSignal: args.abortSignal, + rejectPinned: true, }); journal.push({ ...target, diff --git a/src/node/services/memoryConsolidationService.test.ts b/src/node/services/memoryConsolidationService.test.ts index 4f9fdaca80..f2d31f3bbc 100644 --- a/src/node/services/memoryConsolidationService.test.ts +++ b/src/node/services/memoryConsolidationService.test.ts @@ -204,7 +204,10 @@ interface Fixture extends Disposable { setEnabled: (enabled: boolean) => void; /** When true, scripted runs emit a fatal stream error instead of finishing. */ setStreamFailing: (failing: boolean) => void; - addWorkspace: (id: string, opts?: { archivedAt?: string }) => Promise; + addWorkspace: ( + id: string, + opts?: { archivedAt?: string; parentWorkspaceId?: string } + ) => Promise; addMultiProjectWorkspace: (id: string, opts?: { bucket?: string }) => Promise; } @@ -283,6 +286,7 @@ async function createFixture(options?: { name: id, path: `/projects/demo/${id}`, archivedAt: opts?.archivedAt, + parentWorkspaceId: opts?.parentWorkspaceId, }); return cfg; }); @@ -395,6 +399,30 @@ describe("MemoryConsolidationService", () => { }, }), }); + await fixture.addWorkspace("ws-sub", { parentWorkspaceId: "ws-dream" }); + const childMetadata = await seedCompactionEpoch(fixture, "ws-sub"); + await fsPromises.writeFile( + path.join(fixture.xumHome, "memory-consolidation.json"), + JSON.stringify({ + workspaces: {}, + harvestsByWorkspace: { + "ws-sub": { + [childMetadata.summaryMessageId]: { + status: "failed", + startedAt: Date.now() - 10_000, + completedAt: Date.now() - 9_000, + attemptCount: 1, + boundaryKey: childMetadata.summaryMessageId, + compactionEpoch: childMetadata.compactionEpoch, + acceptedCandidates: 0, + skippedCandidates: 0, + error: "crashed mid-harvest", + completionMetadata: childMetadata, + }, + }, + }, + }) + ); const run = fixture.service.maybeRun("ws-dream", "manual"); await started; await fixture.service.cancelInFlightConsolidation("ws-dream"); @@ -405,10 +433,91 @@ describe("MemoryConsolidationService", () => { if (!result.success) { expect(result.error).toContain("stream failed"); } + // The cancelled owner's continuation must not start recovery of a + // sub-agent's retryable harvest (work outside the drained registry). + const childRecords = ( + JSON.parse( + await fsPromises.readFile(path.join(fixture.xumHome, "memory-consolidation.json"), "utf-8") + ) as { harvestsByWorkspace: Record> } + ).harvestsByWorkspace["ws-sub"]; + expect(Object.values(childRecords).map((record) => record.attemptCount)).toEqual([1]); // Idempotent with nothing in flight (the phantom-metadata removal path). await fixture.service.cancelInFlightConsolidation("ws-dream"); }); + it("a sub-agent's removal drain aborts the owner-keyed run made on its behalf", async () => { + // A child's trigger (and the post-harvest sweep) runs the OWNER's + // consolidation; a cancelled child harvest deliberately falls through to + // that sweep. The child's removal drain must still reach the owner-keyed + // run, or it would keep mutating the shared notebook after removal. + let streamStarted!: () => void; + const started = new Promise((resolve) => (streamStarted = resolve)); + using fixture = await createFixture({ + modelFactory: () => + new MockLanguageModelV3({ + doStream: (options) => { + streamStarted(); + return Promise.resolve({ + stream: new ReadableStream({ + start(controller) { + options.abortSignal?.addEventListener("abort", () => { + controller.error(new Error("request aborted")); + }); + }, + }), + }); + }, + }), + }); + await fixture.addWorkspace("ws-sub", { parentWorkspaceId: "ws-dream" }); + await fixture.addWorkspace("ws-sib", { parentWorkspaceId: "ws-dream" }); + const siblingMetadata = await seedCompactionEpoch(fixture, "ws-sib"); + await fsPromises.writeFile( + path.join(fixture.xumHome, "memory-consolidation.json"), + JSON.stringify({ + workspaces: {}, + harvestsByWorkspace: { + "ws-sib": { + [siblingMetadata.summaryMessageId]: { + status: "failed", + startedAt: Date.now() - 10_000, + completedAt: Date.now() - 9_000, + attemptCount: 1, + boundaryKey: siblingMetadata.summaryMessageId, + compactionEpoch: siblingMetadata.compactionEpoch, + acceptedCandidates: 0, + skippedCandidates: 0, + error: "crashed mid-harvest", + completionMetadata: siblingMetadata, + }, + }, + }, + }) + ); + const run = fixture.service.maybeRun("ws-sub", "manual"); + await started; + await fixture.service.cancelInFlightConsolidation("ws-sub"); + const result = await run; + expect(result.success).toBe(false); + if (!result.success) expect(result.error).toContain("stream failed"); + // The run was keyed by the owner, but the ACTING child is the one being + // torn down: its continuation must not start recovery of a sibling's + // retryable harvest (fresh provider work during the child's teardown). + const siblingRecords = ( + JSON.parse( + await fsPromises.readFile(path.join(fixture.xumHome, "memory-consolidation.json"), "utf-8") + ) as { harvestsByWorkspace: Record> } + ).harvestsByWorkspace["ws-sib"]; + expect(Object.values(siblingRecords).map((record) => record.attemptCount)).toEqual([1]); + // Locally cancelled child: neither its own trigger nor an owner run made + // on its behalf may start while teardown is under way. + const refused = await fixture.service.maybeRun("ws-dream", "manual", { + actingWorkspaceId: "ws-sub", + }); + expect(refused.success).toBe(false); + if (!refused.success) expect(refused.error).toContain("being removed"); + }); + it("runs, persists the journal record, and reports it via getRecord", async () => { using fixture = await createFixture(); const result = await fixture.service.maybeRun("ws-dream", "compaction"); @@ -1280,6 +1389,120 @@ describe("MemoryConsolidationService", () => { expect(fixture.modelCalls).toHaveLength(1); }); + it("recovers a sub-agent's failed harvest through the owner's run", async () => { + using fixture = await createFixture({ modelFactory: harvestCandidateModel }); + await fixture.addWorkspace("ws-sub", { parentWorkspaceId: "ws-dream" }); + const metadata = await seedCompactionEpoch(fixture, "ws-sub"); + await fsPromises.writeFile( + path.join(fixture.xumHome, "memory-consolidation.json"), + JSON.stringify({ + workspaces: {}, + harvestsByWorkspace: { + "ws-sub": { + [metadata.summaryMessageId]: { + status: "failed", + startedAt: Date.now() - 10_000, + completedAt: Date.now() - 9_000, + attemptCount: 1, + boundaryKey: metadata.summaryMessageId, + compactionEpoch: metadata.compactionEpoch, + acceptedCandidates: 0, + skippedCandidates: 0, + error: "crashed mid-harvest", + completionMetadata: metadata, + }, + }, + }, + }) + ); + + // The child's manual run redirects to the owner; the owner's recovery + // must still retry the CHILD's bucket (the launch sweep never visits it). + expect((await fixture.service.maybeRun("ws-sub", "manual")).success).toBe(true); + const status = await fixture.service.getStatus("ws-sub"); + expect(status.latestHarvestRecord?.status).toBe("completed"); + expect(status.latestHarvestRecord?.attemptCount).toBe(2); + }); + + it("redirects a sub-agent's Dream runs and status to the memory owner", async () => { + using fixture = await createFixture(); + await fixture.addWorkspace("ws-sub", { parentWorkspaceId: "ws-dream" }); + + // Launch sweep: a child's workspace write is keyed under the owner + // (MemoryService resolution), so the idle owner qualifies while the idle + // child is skipped even though it is listed. + await fixture.memoryService.create( + { runtime: null, checkoutCwd: "", workspaceId: "ws-sub", projectPath: "" }, + "/memories/workspace/from-child.md", + "shared lesson", + "agent" + ); + const dayAgo = Date.now() - 25 * 60 * 60 * 1000; + // A recently-used child keeps the whole tree's notebook "not idle": the + // owner must not be swept on its own stale recency. + await fixture.service.runLaunchSweep( + new Map([ + ["ws-sub", Date.now()], + ["ws-dream", dayAgo], + ]) + ); + expect(fixture.modelCalls).toHaveLength(0); + await fixture.service.runLaunchSweep( + new Map([ + ["ws-sub", dayAgo], + ["ws-dream", dayAgo], + ]) + ); + expect(fixture.modelCalls).toHaveLength(1); + expect(await fixture.service.getRecord("ws-sub")).toBeNull(); + expect(await fixture.service.getRecord("ws-dream")).not.toBeNull(); + + // Manual/compaction runs from the child consolidate the OWNER's store + // under the owner's lock; the record lands on the owner and the child's + // status view reports it (the tab shows the shared store). + const manual = await fixture.service.maybeRun("ws-sub", "manual"); + expect(manual.success).toBe(true); + expect(fixture.modelCalls).toHaveLength(2); + expect(await fixture.service.getRecord("ws-sub")).toBeNull(); + expect((await fixture.service.getStatus("ws-sub")).workspaceRecord).toEqual( + await fixture.service.getRecord("ws-dream") + ); + + // Archive is the owner's own one-shot promotion pass: a child archive is + // refused instead of running it. + const archive = await fixture.service.maybeRun("ws-sub", "archive"); + expect(archive.success).toBe(false); + if (!archive.success) expect(archive.error).toContain("owner"); + expect(fixture.modelCalls).toHaveLength(2); + + // A child mid-teardown must not start an owner run: locally cancelled + // (the child's removal drain could never cancel an owner-keyed run) or + // tombstoned by another backend. + await fixture.addWorkspace("ws-sub-gone", { parentWorkspaceId: "ws-dream" }); + const tombstonePath = workspaceRemovalTombstonePath(fixture.xumHome, "ws-sub-gone"); + await fsPromises.mkdir(path.dirname(tombstonePath), { recursive: true }); + await fsPromises.writeFile( + tombstonePath, + JSON.stringify({ workspaceId: "ws-sub-gone", removedAt: Date.now() }) + ); + const tombstoned = await fixture.service.maybeRun("ws-sub-gone", "manual"); + expect(tombstoned.success).toBe(false); + if (!tombstoned.success) expect(tombstoned.error).toContain("being removed"); + await fixture.service.cancelInFlightConsolidation("ws-sub"); + const cancelled = await fixture.service.maybeRun("ws-sub", "manual"); + expect(cancelled.success).toBe(false); + if (!cancelled.success) expect(cancelled.error).toContain("being removed"); + expect(fixture.modelCalls).toHaveLength(2); + + // A dangling parent chain resolves to a PRIVATE store (owner == self), so + // that workspace consolidates itself rather than being orphaned forever. + await fixture.addWorkspace("ws-orphan", { parentWorkspaceId: "ws-gone" }); + const orphan = await fixture.service.maybeRun("ws-orphan", "manual"); + expect(orphan.success).toBe(true); + expect(await fixture.service.getRecord("ws-orphan")).not.toBeNull(); + expect(fixture.modelCalls).toHaveLength(3); + }); + it("launch sweep skips archived workspaces and caps runs per launch", async () => { using fixture = await createFixture(); const dayAgo = Date.now() - 25 * 60 * 60 * 1000; diff --git a/src/node/services/memoryConsolidationService.ts b/src/node/services/memoryConsolidationService.ts index 9e830b3ccb..33fe367630 100644 --- a/src/node/services/memoryConsolidationService.ts +++ b/src/node/services/memoryConsolidationService.ts @@ -93,6 +93,12 @@ interface MemoryConsolidationRunOptions { */ skipWorkspaceDebounce?: boolean; skipHarvestRecovery?: boolean; + /** + * Set by maybeRun when a sub-agent's trigger is redirected to its owner: the + * child's own durable removal tombstone then gates the owner run too, since + * a child mid-teardown must not mutate the shared notebook. + */ + actingWorkspaceId?: string; } interface ExperimentsCheck { @@ -414,8 +420,12 @@ export class MemoryConsolidationService extends EventEmitter { const workspace = self.config.findWorkspace(workspaceId); const projectPath = workspace == null ? "" : resolveConsolidationProjectPath(workspace); const globalRecord = findNewestWorkspaceRecord(file.workspaces); + // The workspace record describes the STORE the tab shows: for a + // sub-agent that is the owner's (runs are redirected there, see + // maybeRun); harvests stay per acting workspace. + const ownerWorkspaceId = self.memoryService.resolveWorkspaceMemoryOwnerId(workspaceId); return { - workspaceRecord: file.workspaces[workspaceId] ?? null, + workspaceRecord: file.workspaces[ownerWorkspaceId] ?? null, projectRecord: projectPath === "" ? null : (file.projects[projectPath] ?? null), globalRecord, latestHarvestRecord: findNewestHarvestRecord(file.harvestsByWorkspace[workspaceId]), @@ -494,10 +504,22 @@ export class MemoryConsolidationService extends EventEmitter { const self = this; return Effect.gen(function* () { const sidecar = yield* self.loadEffect(); - const records = sidecar.harvestsByWorkspace[workspaceId]; - if (records === undefined) return; + // Harvest buckets are keyed by the ACTING workspace, but a sub-agent's + // runs redirect to its owner (see maybeRun) and the launch sweep skips + // children, so an owner run must retry every tree member's bucket or a + // child's failed/stale harvest would never be recovered. + const cfg = self.config.loadConfigOrDefault(); + const records = Object.entries(sidecar.harvestsByWorkspace) + .filter( + ([bucketId]) => + !self.removalCancelled.has(bucketId) && + (bucketId === workspaceId || + self.memoryService.resolveWorkspaceMemoryOwnerId(bucketId, () => cfg) === workspaceId) + ) + .flatMap(([, bucket]) => Object.values(bucket)); + if (records.length === 0) return; - const retryable = Object.values(records) + const retryable = records .filter((record) => { if (record.completionMetadata === undefined) return false; if (record.attemptCount >= HARVEST_MAX_ATTEMPTS) return false; @@ -542,26 +564,48 @@ export class MemoryConsolidationService extends EventEmitter { return Effect.promise(() => isWorkspaceRemovalTombstoned(this.config.rootDir, workspaceId)); } - /** Register one run's removal controller; disposed when the run settles. */ - private trackRunController(workspaceId: string): { + /** + * Register one run's removal controller; disposed when the run settles. + * A run made on a sub-agent's behalf over the OWNER's store (a redirected + * trigger, the post-harvest sweep) is registered under the acting child's + * id too: the child's removal drain (cancelInFlightConsolidation) must abort + * it — a cancelled child harvest deliberately falls through to this sweep, + * which could otherwise keep mutating the shared notebook for its whole + * run after the removal's bounded drain returned. + */ + private trackRunController( + workspaceId: string, + actingWorkspaceId?: string + ): { controller: AbortController; dispose: () => void; } { const controller = new AbortController(); + const ids = [ + workspaceId, + ...(actingWorkspaceId !== undefined && actingWorkspaceId !== workspaceId + ? [actingWorkspaceId] + : []), + ]; // r61: a run registered after teardown began (follow-on sweep/recovery // racing the entry checks) must start already aborted. - if (this.removalCancelled.has(workspaceId)) { + if (ids.some((id) => this.removalCancelled.has(id))) { controller.abort(); } - const set = this.runControllers.get(workspaceId) ?? new Set(); - set.add(controller); - this.runControllers.set(workspaceId, set); + const sets = ids.map((id) => { + const set = this.runControllers.get(id) ?? new Set(); + set.add(controller); + this.runControllers.set(id, set); + return [id, set] as const; + }); return { controller, dispose: () => { - set.delete(controller); - if (set.size === 0 && this.runControllers.get(workspaceId) === set) { - this.runControllers.delete(workspaceId); + for (const [id, set] of sets) { + set.delete(controller); + if (set.size === 0 && this.runControllers.get(id) === set) { + this.runControllers.delete(id); + } } }, }; @@ -667,9 +711,41 @@ export class MemoryConsolidationService extends EventEmitter { options: MemoryConsolidationRunOptions = {} ): Promise> { if (!this.enabled()) return Err("memory-consolidation experiment is disabled"); - if (this.removalCancelled.has(workspaceId)) { + // Sub-agents share their task-tree owner's /memories/workspace store + // (MemoryService.resolveWorkspaceMemoryOwnerId): a child's manual or + // post-compaction run consolidates the OWNER's notebook under the owner's + // in-flight lock, so it never races the owner's own runs and the child's + // harvested candidates are actually swept. Archive is the owner's own + // one-shot promotion pass; a child archive must not trigger it. Compared + // by resolved owner, not parentWorkspaceId: a dangling/cyclic chain falls + // back to a private store that must stay consolidatable. + // + // The acting child's own teardown gate comes BEFORE the redirect: + // cancelInFlightConsolidation(child) marks only the child id, and an + // owner-keyed run reserved after that would neither be refused by the + // owner's check below nor be cancellable by the child's removal drain. + if ( + this.removalCancelled.has(workspaceId) || + (options.actingWorkspaceId !== undefined && + this.removalCancelled.has(options.actingWorkspaceId)) + ) { return Err("workspace is being removed; consolidation refused"); } + const ownerWorkspaceId = this.memoryService.resolveWorkspaceMemoryOwnerId(workspaceId); + if (ownerWorkspaceId !== workspaceId) { + if (trigger === "archive") { + return Err( + "sub-agent workspaces share their owner's workspace memory; the owner's archive pass promotes it" + ); + } + // The owner run's recovery covers this child's harvest bucket too. The + // child's durable tombstone is probed behind the reservation, like the + // owner's (actingWorkspaceId). + return this.maybeRun(ownerWorkspaceId, trigger, { + ...options, + actingWorkspaceId: options.actingWorkspaceId ?? workspaceId, + }); + } const active = this.inFlight.get(workspaceId); if (active !== undefined) { // Archive is the workspace's one-shot final pass (workspace→global @@ -686,7 +762,7 @@ export class MemoryConsolidationService extends EventEmitter { // would otherwise also pass the check and start a second concurrent run // over the same directories. runPromise starts the fiber synchronously, // and the map is populated before this frame yields either way. - const removal = this.trackRunController(workspaceId); + const removal = this.trackRunController(workspaceId, options.actingWorkspaceId); const run = Effect.runPromise( this.runLockedEffect(workspaceId, trigger, options, removal.controller.signal) ); @@ -698,7 +774,17 @@ export class MemoryConsolidationService extends EventEmitter { this.inFlight.delete(workspaceId); removal.dispose(); } - if (options.skipHarvestRecovery !== true) { + // Removal cancelled this run: recovery would spawn child harvests outside + // the cancellation registry being drained (their controllers were never + // registered), mutating the shared inbox during destructive teardown. + // A child-initiated run is keyed by the OWNER, so the acting child's own + // cancellation must gate it too. + if ( + options.skipHarvestRecovery !== true && + !this.removalCancelled.has(workspaceId) && + (options.actingWorkspaceId === undefined || + !this.removalCancelled.has(options.actingWorkspaceId)) + ) { await Effect.runPromise(this.recoverRetryableHarvestsEffect(workspaceId)); } return result; @@ -719,6 +805,12 @@ export class MemoryConsolidationService extends EventEmitter { if (yield* self.isRemovalTombstonedEffect(workspaceId)) { return Err("workspace is being removed; consolidation refused"); } + if ( + options.actingWorkspaceId !== undefined && + (yield* self.isRemovalTombstonedEffect(options.actingWorkspaceId)) + ) { + return Err("workspace is being removed; consolidation refused"); + } // Manual runs bypass debounce (an explicit /dream is explicit intent). // Archive too: it is the workspace's one-shot final pass — the only @@ -733,6 +825,11 @@ export class MemoryConsolidationService extends EventEmitter { const workspace = self.config.findWorkspace(workspaceId); if (!workspace) return Err(`workspace not found: ${workspaceId}`); + // Sub-agents share their task-tree owner's /memories/workspace store + // (MemoryService.resolveWorkspaceMemoryOwnerId), so a Dream run from a + // child would consolidate the owner's notebook concurrently with the + // owner's own runs. Children still harvest into the shared inbox; only + // the owner sweeps it. const agentBody = yield* Effect.promise(() => resolveDreamAgentBody(self.config.rootDir)); if (agentBody === null) return Err("dream agent definition is missing"); @@ -957,7 +1054,16 @@ export class MemoryConsolidationService extends EventEmitter { .pipe(Effect.catch(journalHarvestFailure), Effect.catchDefect(journalHarvestFailure)); } - return yield* Effect.promise(() => self.runCompactionSweepAfterHarvest(metadata.workspaceId)); + // A sub-agent's inbox lives in the owner's store: wait on and run the + // OWNER's consolidation (see maybeRun) so the harvest is actually swept — + // still as the acting CHILD, so the child's in-process cancellation, + // durable tombstone and removal drain bind the owner-keyed run too. + return yield* Effect.promise(() => + self.runCompactionSweepAfterHarvest( + self.memoryService.resolveWorkspaceMemoryOwnerId(metadata.workspaceId), + metadata.workspaceId + ) + ); }); } @@ -1069,7 +1175,8 @@ export class MemoryConsolidationService extends EventEmitter { } private async runCompactionSweepAfterHarvest( - workspaceId: string + workspaceId: string, + actingWorkspaceId: string ): Promise> { for (;;) { const active = this.inFlight.get(workspaceId); @@ -1082,6 +1189,7 @@ export class MemoryConsolidationService extends EventEmitter { const result = await this.maybeRun(workspaceId, trigger, { skipWorkspaceDebounce: true, skipHarvestRecovery: true, + ...(actingWorkspaceId !== workspaceId ? { actingWorkspaceId } : {}), }); if (!result.success && result.error === "a consolidation run is already in flight") { continue; @@ -1193,7 +1301,8 @@ export class MemoryConsolidationService extends EventEmitter { let globalLastRunAt = findNewestWorkspaceRecord(sidecar.workspaces)?.lastRunAt ?? 0; const archivedById = new Map(); const projectPathByWorkspace = new Map(); - for (const [configProjectPath, project] of self.config.loadConfigOrDefault().projects) { + const cfg = self.config.loadConfigOrDefault(); + for (const [configProjectPath, project] of cfg.projects) { for (const workspace of project.workspaces) { if (workspace.id === undefined) continue; archivedById.set( @@ -1216,8 +1325,18 @@ export class MemoryConsolidationService extends EventEmitter { ]) ); - let started = 0; + // Shared stores: a child's writes are keyed under its owner, so the + // owner's row absorbs every tree member's recency (a notebook is idle + // only when the WHOLE tree is) and child rows are then dropped — + // running a child would just redirect to the owner anyway. + const treeRecency = new Map(); for (const [workspaceId, recency] of recencyByWorkspace) { + const ownerId = self.memoryService.resolveWorkspaceMemoryOwnerId(workspaceId, () => cfg); + treeRecency.set(ownerId, Math.max(treeRecency.get(ownerId) ?? 0, recency)); + } + + let started = 0; + for (const [workspaceId, recency] of treeRecency) { if (started >= MEMORY_CONSOLIDATION_LAUNCH_SWEEP_CAP) break; if (now - recency < MEMORY_CONSOLIDATION_IDLE_MS) continue; if (archivedById.get(workspaceId) === true) continue; diff --git a/src/node/services/memoryOperations.test.ts b/src/node/services/memoryOperations.test.ts index deee04bd98..20ad73f028 100644 --- a/src/node/services/memoryOperations.test.ts +++ b/src/node/services/memoryOperations.test.ts @@ -198,6 +198,33 @@ describe("memory operations", () => { expect(await memoryMetaService.getPinnedKeys()).toEqual(new Set(["global:prefs.md"])); }); + test("setPinned broadcasts a change event so other tabs on the same store refetch", async () => { + const context = createContext({ enabled: true }); + await saveMemory(context, { + workspaceId: "ws-mem", + path: "/memories/workspace/pinned.md", + content: "x", + expectedSha256: null, + }); + const events: MemoryChangeEvent[] = []; + memoryService.on("change", (event: MemoryChangeEvent) => events.push(event)); + const result = await setMemoryPinned(context, { + workspaceId: "ws-mem", + path: "/memories/workspace/pinned.md", + pinned: true, + }); + expect(result).toEqual({ success: true, data: undefined }); + expect(events).toEqual([ + { + scope: "workspace", + path: "/memories/workspace/pinned.md", + actor: "user", + workspaceId: "ws-mem", + projectPath, + }, + ]); + }); + test("list exposes usage stats; UI reads do not count as uses", async () => { const client = createClient({ enabled: true }); await client.memory.save({ diff --git a/src/node/services/memoryOperations.ts b/src/node/services/memoryOperations.ts index 0837e5250b..ec9612d699 100644 --- a/src/node/services/memoryOperations.ts +++ b/src/node/services/memoryOperations.ts @@ -23,7 +23,7 @@ import { resolveMemoryProjectIdentity, type MemoryScopeContext, } from "./memoryService"; -import { memoryLogicalKey } from "./memoryMeta"; +import { MemoryMetaWriteError, memoryLogicalKey } from "./memoryMeta"; type MemoryContext = Pick< ORPCContext, @@ -57,6 +57,8 @@ function workspaceNotFound(workspaceId: string | null | undefined): string { interface ResolvedMemoryScope { projectPath: string; + /** Task-tree root whose store backs workspace scope ("" without a workspace); keys sidecar pins/stats. */ + ownerWorkspaceId: string; scopeCtx: MemoryScopeContext; } @@ -73,19 +75,24 @@ function resolveMemoryScope( if (workspaceId == null) return { projectPath: "", + ownerWorkspaceId: "", scopeCtx: { runtime: null, checkoutCwd: "", workspaceId: "", projectPath: "" }, }; const metadata = yield* Effect.promise(() => context.workspaceService.getInfo(workspaceId)); if (!metadata) return yield* Effect.fail(new MemoryWorkspaceNotFoundError({ workspaceId })); const projectPath = resolveMemoryProjectIdentity(metadata); + const scopeCtx: MemoryScopeContext = { + runtime: createRuntimeForWorkspace(metadata), + checkoutCwd: "", + workspaceId, + projectPath, + }; return { projectPath, - scopeCtx: { - runtime: createRuntimeForWorkspace(metadata), - checkoutCwd: "", - workspaceId, - projectPath, - }, + // Same per-context resolution the store/notify paths use, so sidecar keys + // and the physical store never disagree about the owner within a request. + ownerWorkspaceId: context.memoryService.ownerWorkspaceIdFor(scopeCtx), + scopeCtx, }; }); } @@ -102,7 +109,7 @@ export function listMemoryEffect(context: MemoryContext, input: Input context.memoryService.setPinned(resolved.scopeCtx, input.path, input.pinned), + catch: (error: unknown) => error, + }).pipe( + Effect.map(() => ({ success: true as const, data: undefined })), + // Sidecar write failures (disk full, permissions) arrive as the typed + // MemoryMetaWriteError; lock timeouts and command errors map onto the + // same legacy string error channel instead of escaping as an untyped + // INTERNAL_SERVER_ERROR rejection. + Effect.catch((error) => + Effect.succeed({ + success: false as const, + error: + error instanceof MemoryMetaWriteError + ? `Failed to persist pin state: ${error.reason}` + : `Failed to update pin: ${getErrorMessage(error)}`, + }) ) - .pipe( - Effect.map(() => ({ success: true as const, data: undefined })), - // Sidecar write failures (disk full, permissions) arrive as the typed - // MemoryMetaWriteError and map onto the legacy string error channel - // instead of escaping as an untyped INTERNAL_SERVER_ERROR rejection. - Effect.catchTag("MemoryMetaWriteError", (error) => - Effect.succeed({ - success: false as const, - error: `Failed to persist pin state: ${error.reason}`, - }) - ) - ); + ); }).pipe(Effect.catchTag("MemoryWorkspaceNotFoundError", workspaceNotFoundAsStringError)); } diff --git a/src/node/services/memoryService.test.ts b/src/node/services/memoryService.test.ts index a71d2e2fe5..f8d7bc5f8a 100644 --- a/src/node/services/memoryService.test.ts +++ b/src/node/services/memoryService.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect } from "bun:test"; +import { describe, it, expect, spyOn } from "bun:test"; import { MEMORY_MAX_FILES_PER_SCOPE, MEMORY_MAX_FILE_BYTES } from "@/common/constants/memory"; @@ -16,7 +16,7 @@ import { type MemoryScopeContext, type PinnedFileMutation, } from "./memoryService"; -import { MemoryMetaService } from "./memoryMeta"; +import { MemoryMetaService, memoryLogicalKey } from "./memoryMeta"; import { MemoryRefinementActionSchema, REFINEMENT_CAPTURE_MAX_FILES, @@ -24,6 +24,8 @@ import { RefinementInverseSchema, } from "@/common/types/refinement"; import { applyRefinementInverse, readRefinementEvents } from "./refinement/refinementTestHelpers"; +import { rollbackRefinement } from "./refinement/refinementRollback"; +import { workspaceRemovalTombstonePath } from "./workspaceRemoval"; import { TestTempDir } from "./tools/testHelpers"; function pathExists(target: string): Promise { @@ -863,6 +865,387 @@ describe("MemoryService", () => { }); }); + describe("sub-agent workspace memory sharing", () => { + /** Register owner → child → grandchild so parentWorkspaceId chains resolve. */ + async function registerTaskTree(fixture: MemoryFixture): Promise { + await fixture.config.editConfig((cfg) => { + cfg.projects.set(FIXTURE_PROJECT_PATH, { + workspaces: [ + { id: "ws-owner", name: "owner", path: "/checkouts/owner" }, + { + id: "ws-child", + name: "child", + path: "/checkouts/child", + parentWorkspaceId: "ws-owner", + }, + { + id: "ws-grandchild", + name: "grandchild", + path: "/checkouts/grandchild", + parentWorkspaceId: "ws-child", + }, + { id: "ws-solo", name: "solo", path: "/checkouts/solo" }, + ], + }); + return cfg; + }); + } + + it("resolves the task-tree root as the owner; unknown and parentless ids resolve to themselves", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-owner")).toBe("ws-owner"); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-child")).toBe("ws-owner"); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-grandchild")).toBe("ws-owner"); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-solo")).toBe("ws-solo"); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-unregistered")).toBe( + "ws-unregistered" + ); + }); + + it("resolves from a caller snapshot without touching the config file", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + const cfg = fixture.config.loadConfigOrDefault(); + const stamp = spyOn(fixture.config, "configFileStamp"); + const load = spyOn(fixture.config, "loadConfigOrDefault"); + // Bulk passes (launch sweep over every recorded workspace) must not pay + // one synchronous stat per workspace on the main process. + for (const id of ["ws-owner", "ws-child", "ws-grandchild", "ws-solo"]) { + fixture.service.resolveWorkspaceMemoryOwnerId(id, () => cfg); + } + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-grandchild", () => cfg)).toBe( + "ws-owner" + ); + expect(stamp).not.toHaveBeenCalled(); + expect(load).not.toHaveBeenCalled(); + }); + + it("stores a sub-agent's workspace notes in the owner's session dir, visible to the whole tree", async () => { + using fixture = await createFixture("ws-grandchild"); + await registerTaskTree(fixture); + const events: unknown[] = []; + fixture.service.on("change", (event) => events.push(event)); + + const created = await fixture.service.create( + fixture.ctx, + "/memories/workspace/context-notes.md", + "found the bug in parser.ts", + "agent" + ); + expect(created.success).toBe(true); + + // Physically in the OWNER's session dir, not the grandchild's. + const ownerPhysical = path.join( + fixture.config.sessionsDir, + "ws-owner", + "memory", + "context-notes.md" + ); + expect(await fsPromises.readFile(ownerPhysical, "utf-8")).toBe("found the bug in parser.ts"); + expect( + await pathExists(path.join(fixture.config.sessionsDir, "ws-grandchild", "memory")) + ).toBe(false); + + // Owner and sibling child read the same file through their own contexts. + for (const workspaceId of ["ws-owner", "ws-child"]) { + const viewed = await fixture.service.view( + { ...fixture.ctx, workspaceId }, + "/memories/workspace/context-notes.md" + ); + expect(viewed.success).toBe(true); + if (viewed.success) expect(viewed.output).toContain("found the bug in parser.ts"); + } + // An unrelated workspace does not see it. + const solo = await fixture.service.view( + { ...fixture.ctx, workspaceId: "ws-solo" }, + "/memories/workspace/context-notes.md" + ); + expect(solo.success).toBe(false); + + // Change events name the owner so the owner's Memory tab (and every + // tree member's) refreshes; sidecar stats are keyed by the owner too. + // The two tree-member views publish as well: a read re-ranks the shared + // hot set, so sibling sessions must drop their cached memory context. + const ownerEvent = { + scope: "workspace", + path: "/memories/workspace/context-notes.md", + actor: "agent", + workspaceId: "ws-owner", + projectPath: FIXTURE_PROJECT_PATH, + }; + expect(events).toEqual([ownerEvent, ownerEvent, ownerEvent]); + const meta = await fixture.metaService.getEntries(); + expect( + meta.get( + memoryLogicalKey("workspace", "context-notes.md", { + projectPath: "", + workspaceId: "ws-owner", + }) + )?.lastWriteAt + ).not.toBeNull(); + expect( + meta.has( + memoryLogicalKey("workspace", "context-notes.md", { + projectPath: "", + workspaceId: "ws-grandchild", + }) + ) + ).toBe(false); + }); + + it("refuses a sub-agent's mutation once the owner's removal tombstone exists", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + const tombstone = workspaceRemovalTombstonePath(fixture.config.rootDir, "ws-owner"); + await fsPromises.mkdir(path.dirname(tombstone), { recursive: true }); + await fsPromises.writeFile(tombstone, ""); + + // The child itself is alive, but its notebook is the removed owner's: + // committing would recreate the deleted owner directory. + const created = await fixture.service.create( + fixture.ctx, + "/memories/workspace/n.md", + "shared", + "agent" + ); + expect(created.success).toBe(false); + if (!created.success) expect(created.error).toContain("ws-owner was removed"); + expect( + await pathExists(path.join(fixture.config.sessionsDir, "ws-owner", "memory", "n.md")) + ).toBe(false); + // Global scope is not the owner's store and stays writable. + const globalCreate = await fixture.service.create( + fixture.ctx, + "/memories/global/n.md", + "mine", + "agent" + ); + expect(globalCreate.success).toBe(true); + }); + + it("journals a sub-agent's workspace-scope mutation in its own session", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + const created = await fixture.service.create( + fixture.ctx, + "/memories/workspace/n.md", + "shared", + "agent" + ); + expect(created.success).toBe(true); + + // Attribution stays with the acting workspace: the row is in the + // child's journal but its inverse points into the owner's memory dir. + const childSessionDir = path.join(fixture.config.sessionsDir, "ws-child"); + const ownerSessionDir = path.join(fixture.config.sessionsDir, "ws-owner"); + const events = await readRefinementEvents(childSessionDir); + expect(events).toHaveLength(1); + expect(await readRefinementEvents(ownerSessionDir)).toHaveLength(0); + const physical = path.join(ownerSessionDir, "memory", "n.md"); + expect(events[0].data.inverse).toEqual({ op: "delete-files", paths: [physical] }); + + // Confinement: the child's own memory root does not admit the path. + const refused = await rollbackRefinement({ + sessionDir: childSessionDir, + id: events[0].id, + evidence: { toolName: "test", actor: "user" }, + }); + expect(refused.success).toBe(false); + if (!refused.success) expect(refused.error).toContain("outside every memory scope root"); + expect(await pathExists(physical)).toBe(true); + }); + + it("re-resolves the owner after config changes so a removed owner's child falls back to its own store", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-child")).toBe("ws-owner"); + const invalidated: string[][] = []; + fixture.service.on("ownersInvalidated", (ids: string[]) => invalidated.push(ids)); + + // The owner is deregistered (removal with a live shared-checkout child); + // the dangling chain must not keep pointing at the removed owner. + await fixture.config.editConfig((cfg) => { + const project = cfg.projects.get(FIXTURE_PROJECT_PATH)!; + project.workspaces = project.workspaces.filter((ws) => ws.id !== "ws-owner"); + return cfg; + }); + // Live sessions of formerly-shared children are told to drop their cache. + expect(invalidated).toEqual([["ws-child"]]); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-child")).toBe("ws-child"); + const created = await fixture.service.create( + fixture.ctx, + "/memories/workspace/after.md", + "own store now", + "agent" + ); + expect(created.success).toBe(true); + expect( + await pathExists(path.join(fixture.config.sessionsDir, "ws-child", "memory", "after.md")) + ).toBe(true); + }); + + it("ignores config edits that leave the memory topology unchanged", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-child")).toBe("ws-owner"); + const invalidated: string[][] = []; + fixture.service.on("ownersInvalidated", (ids: string[]) => invalidated.push(ids)); + + // Ordinary churn (a retitle) must not make every live child rebuild its + // memory context, and the unchanged mapping stays memoized. + await fixture.config.editConfig((cfg) => { + const project = cfg.projects.get(FIXTURE_PROJECT_PATH)!; + project.workspaces.find((ws) => ws.id === "ws-child")!.title = "renamed"; + return cfg; + }); + expect(invalidated).toEqual([]); + const load = spyOn(fixture.config, "loadConfigOrDefault"); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-child")).toBe("ws-owner"); + expect(load).not.toHaveBeenCalled(); + }); + + it("re-resolves the owner after an EXTERNAL config rewrite (another backend removed it)", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-child")).toBe("ws-owner"); + + // Rewrite config.json directly: no local onConfigChanged fires, only the + // file's stamp changes — as when a second backend deregisters the owner. + const configFile = path.join(fixture.xumHome, "config.json"); + // On disk, projects are [path, project] tuples. + const raw = JSON.parse(await fsPromises.readFile(configFile, "utf-8")) as { + projects: Array<[string, { workspaces: Array<{ id: string }> }]>; + }; + const project = raw.projects.find(([projectPath]) => projectPath === FIXTURE_PROJECT_PATH); + expect(project).toBeDefined(); + project![1].workspaces = project![1].workspaces.filter((ws) => ws.id !== "ws-owner"); + await fsPromises.writeFile(configFile, JSON.stringify(raw, null, 2)); + // Same-tick same-size rewrites can leave mtime unchanged; force a distinct stamp. + await fsPromises.utimes( + configFile, + new Date(Date.now() + 5_000), + new Date(Date.now() + 5_000) + ); + + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-child")).toBe("ws-child"); + }); + + it("announces the self→shared transition when config.json recovers after being unreadable", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + const invalidated: string[][] = []; + fixture.service.on("ownersInvalidated", (ids: string[]) => invalidated.push(ids)); + + // config.json vanishes (another backend mid-rewrite): the child cannot + // resolve its tree and falls back to its private store... + const configFile = path.join(fixture.xumHome, "config.json"); + const parked = `${configFile}.parked`; + await fsPromises.rename(configFile, parked); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-child")).toBe("ws-child"); + expect(invalidated).toEqual([]); + + // ...and once it is back, sessions that built a context on the fallback + // store must be told, even though no shared mapping was ever memoized. + await fsPromises.rename(parked, configFile); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-child")).toBe("ws-owner"); + expect(invalidated).toEqual([["ws-child"]]); + }); + + it("does not memoize the self fallback taken while config.json is unreadable but unchanged", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + // The file stats the same (no stamp change) but cannot be read/parsed + // for a moment (EACCES interval, non-atomic writer): the lenient load + // yields the empty default while the strict one throws. + const real = fixture.config.loadConfigOrDefault.bind(fixture.config); + const unreadable = spyOn(fixture.config, "loadConfigOrDefault").mockImplementation( + (options?: { throwOnError?: boolean }) => { + if (options?.throwOnError) throw new Error("EACCES: permission denied"); + return { ...real(), projects: new Map() }; + } + ); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-child")).toBe("ws-child"); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-child")).toBe("ws-child"); + // Readability returns without the stamp moving: the next resolution + // must see the real tree instead of a pinned fallback. + unreadable.mockRestore(); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-child")).toBe("ws-owner"); + }); + + it("keeps the owner memo retryable when a local config edit notifies while the file is unreadable", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-child")).toBe("ws-owner"); + // A local edit removes the owner. The change notification fires while + // the file cannot be read (a swallowed late write failure): the memo + // must not be stamped as current, or the stale mapping survives until + // an unrelated rewrite once readability returns without a stamp change. + const real = fixture.config.loadConfigOrDefault.bind(fixture.config); + const unreadable = spyOn(fixture.config, "loadConfigOrDefault").mockImplementation( + (options?: { throwOnError?: boolean }) => { + if (options?.throwOnError) throw new Error("EACCES: permission denied"); + return { ...real(), projects: new Map() }; + } + ); + await fixture.config.editConfig((cfg) => { + for (const project of cfg.projects.values()) { + project.workspaces = project.workspaces.filter((ws) => ws.id !== "ws-owner"); + } + return cfg; + }); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-child")).toBe("ws-owner"); + unreadable.mockRestore(); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-child")).toBe("ws-child"); + }); + + it("refuses to commit into a self-fallback store once config.json has recovered", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + // Mid-command race: the command resolves its store while config.json is + // unreadable (self-fallback) and the file recovers before the commit + // check inside the mutation lock. Only the command's FIRST resolution + // is faked; the pre-commit re-resolution sees the recovered tree. + spyOn(fixture.service, "resolveWorkspaceMemoryOwnerId").mockImplementationOnce( + () => "ws-child" + ); + const created = await fixture.service.create( + fixture.ctx, + "/memories/workspace/late.md", + "x", + "agent" + ); + expect(created.success).toBe(false); + if (!created.success) expect(created.error).toContain("Ownership of the workspace notebook"); + expect( + await pathExists(path.join(fixture.config.sessionsDir, "ws-child", "memory", "late.md")) + ).toBe(false); + expect( + await pathExists(path.join(fixture.config.sessionsDir, "ws-owner", "memory", "late.md")) + ).toBe(false); + }); + + it("keeps memoized owners when a changed config.json cannot be read", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-child")).toBe("ws-owner"); + // The stamp moves (a rewrite) but the contents are unreadable for a + // moment: the memo must not be replaced by the empty default's self + // fallbacks, and the pass must be retried once readable. + const real = fixture.config.loadConfigOrDefault.bind(fixture.config); + const unreadable = spyOn(fixture.config, "loadConfigOrDefault").mockImplementation( + (options?: { throwOnError?: boolean }) => { + if (options?.throwOnError) throw new Error("EACCES: permission denied"); + return { ...real(), projects: new Map() }; + } + ); + spyOn(fixture.config, "configFileStamp").mockReturnValue("rewritten"); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-child")).toBe("ws-owner"); + unreadable.mockRestore(); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-child")).toBe("ws-owner"); + }); + }); + describe("memory index entries", () => { it("lists files across scopes with sanitized frontmatter descriptions", async () => { using fixture = await createFixture(); diff --git a/src/node/services/memoryService.ts b/src/node/services/memoryService.ts index f1078a5481..b23c08f4d3 100644 --- a/src/node/services/memoryService.ts +++ b/src/node/services/memoryService.ts @@ -47,6 +47,10 @@ import { withTargetMutationLock, } from "@/node/services/refinement/targetMutationLocks"; import { memoryLogicalKey, type MemoryMetaService } from "@/node/services/memoryMeta"; +import { + resolveWorkspaceMemoryOwnerId, + workspaceMemoryOwnerResolver, +} from "@/node/services/memoryWorkspaceOwner"; import { REFINEMENT_CAPTURE_MAX_FILES, REFINEMENT_CAPTURE_MAX_TOTAL_BYTES, @@ -71,7 +75,12 @@ export interface MemoryScopeContext { runtime: Runtime | null; /** Workspace checkout cwd. Kept in the context shape for existing callers; storage ignores it. */ checkoutCwd: string; - /** Workspace ID; workspace scope root is /memory. */ + /** + * ACTING workspace ID. The workspace scope root is the memory OWNER's + * /memory, where the owner is the task-tree root: sub-agent + * children (parentWorkspaceId set) share their parent's workspace notes + * (see MemoryService.resolveWorkspaceMemoryOwnerId). + */ workspaceId: string; /** * Stable project identity from Xum config (the project root path, never the @@ -331,7 +340,7 @@ export function projectMemoryDirName(projectPath: string): string { return `${base}-${hash}`; } -function toVirtualPath(scope: MemoryScope, relPath: string): string { +export function toVirtualPath(scope: MemoryScope, relPath: string): string { return relPath === "" ? `${MEMORY_VIRTUAL_ROOT}/${scope}` : `${MEMORY_VIRTUAL_ROOT}/${scope}/${relPath}`; @@ -599,6 +608,22 @@ export class MemoryService extends EventEmitter { private readonly metaService: MemoryMetaService ) { super(); + // Parent links are immutable, but an OWNER can be removed while a + // shared-checkout descendant keeps running; its deregistration lands as a + // config change, after which the child must re-resolve (and fall back to + // its own store) instead of writing into the tombstoned owner forever. + // Local edits notify here; edits by ANOTHER backend (multi-instance) are + // caught by the config-file stamp check in resolveWorkspaceMemoryOwnerId. + // The notification fires after the file write, so adopting the new stamp + // here keeps the next resolve from repeating the invalidation — but only + // once the memo was actually rebuilt from the file: an unreadable file at + // notification time (a swallowed late write failure, an EACCES interval) + // keeps the old stamp so the next resolve retries, exactly like the + // stamp check in resolveWorkspaceMemoryOwnerId. + this.config.onConfigChanged(() => { + const stamp = this.config.configFileStamp(); + if (this.invalidateWorkspaceMemoryOwnerMemo()) this.workspaceMemoryOwnerConfigStamp = stamp; + }); } // ------------------------------------------------------------------------- @@ -608,15 +633,172 @@ export class MemoryService extends EventEmitter { // Best-effort: stats failures must never break a memory command. // ------------------------------------------------------------------------- + /** + * Memo of every owner this process resolved (self-resolutions included), + * valid for one config-file stamp: a workspace's parentWorkspaceId is fixed + * at creation and IDs are never reused, so a mapping can only change through + * a config rewrite, which the stamp check / onConfigChanged catch. Fallback + * observations (unregistered ID, config missing or malformed → self) are + * memoized too, so their recovery to a shared owner is a visible transition + * (see invalidateWorkspaceMemoryOwnerMemo). + */ + private readonly workspaceMemoryOwnerById = new Map(); + /** Config-file stamp (Config.configFileStamp) the memo was built against. */ + private workspaceMemoryOwnerConfigStamp: string | null = null; + + /** + * Memoized resolveWorkspaceMemoryOwnerId (see memoryWorkspaceOwner.ts). The + * config is only loaded on a memo miss. Callers resolving many workspaces + * in one synchronous pass (launch sweep, recovery, config-change diffing) + * supply a shared `snapshot`, which bypasses the memo entirely: neither the + * per-call config stat (O(n) synchronous statSync on the main process for + * n workspaces) nor memoization apply — the snapshot can predate the current + * file stamp (another backend rewriting config.json mid-pass) and would + * otherwise be cached under the newer stamp. + */ + resolveWorkspaceMemoryOwnerId( + workspaceId: string, + snapshot?: () => ReturnType + ): string { + if (snapshot !== undefined) return resolveWorkspaceMemoryOwnerId(snapshot(), workspaceId); + const stamp = this.config.configFileStamp(); + if (stamp !== this.workspaceMemoryOwnerConfigStamp) { + // Adopt the new stamp only once its contents were actually read: a + // changed-but-unreadable file (see below) must be retried on the next + // call, not remembered as "seen". + if (this.invalidateWorkspaceMemoryOwnerMemo()) this.workspaceMemoryOwnerConfigStamp = stamp; + } + const cached = this.workspaceMemoryOwnerById.get(workspaceId); + if (cached !== undefined) return cached; + // Only a successful load is memoized. A config.json that stats fine but + // cannot be read or parsed right now (EACCES interval, half-written by a + // non-atomic writer) yields the fresh-install default — the self + // fallback — and the stamp will not move when readability returns, so a + // memo taken now would pin the child to its private notebook until an + // unrelated config rewrite. The fallback is still returned (callers + // degrade to the private store), just re-resolved on the next call. + let cfg: ReturnType; + try { + cfg = this.config.loadConfigOrDefault({ throwOnError: true }); + } catch (error) { + log.debug("[MemoryService] config unreadable; workspace memory owner not memoized", { + workspaceId, + error, + }); + return resolveWorkspaceMemoryOwnerId(this.config.loadConfigOrDefault(), workspaceId); + } + const owner = resolveWorkspaceMemoryOwnerId(cfg, workspaceId); + this.workspaceMemoryOwnerById.set(workspaceId, owner); + return owner; + } + + /** + * Re-resolve every memoized workspace against the current config and tell + * listeners which ones now map to a DIFFERENT owner: their live sessions + * hold a memory context built from the previous store (an owner that was + * just removed — or the private fallback store used while config.json was + * missing/malformed and the tree could not be resolved), so core.ts + * invalidates those caches and the memory subscription refreshes. There is + * no memory-change event for either transition to ride on. + * + * Most config edits (titles, models, task status) leave the topology alone; + * emitting for those would make every live child rebuild its index and hot + * set from disk on ordinary churn, so only real owner changes are reported. + * One parse, only when something was memoized. Returns false when the + * config could not be read (readable stat, unreadable/unparseable content): + * the memoized mappings are RETAINED rather than replaced by the empty + * default's self fallbacks — those would be pinned until the stamp moved, + * which a restored permission bit never does — and the caller keeps the old + * stamp so the pass is retried on the next resolution. + */ + private invalidateWorkspaceMemoryOwnerMemo(): boolean { + if (this.workspaceMemoryOwnerById.size === 0) return true; + // One parse and one ID index for the whole pass (O(n), not O(n²)). + let cfg: ReturnType; + try { + cfg = this.config.loadConfigOrDefault({ throwOnError: true }); + } catch (error) { + log.debug("[MemoryService] config unreadable; keeping memoized workspace memory owners", { + error, + }); + return false; + } + const resolve = workspaceMemoryOwnerResolver(cfg); + const changed: string[] = []; + for (const [workspaceId, previousOwner] of this.workspaceMemoryOwnerById) { + const owner = resolve(workspaceId); + this.workspaceMemoryOwnerById.set(workspaceId, owner); + if (owner !== previousOwner) changed.push(workspaceId); + } + if (changed.length > 0) this.emit("ownersInvalidated", changed); + return true; + } + + /** + * Per-context owner cache: a context object is created per command / per + * index+hot-set build and reused for every entry within it, so the stamp + * stat behind resolveWorkspaceMemoryOwnerId runs once per operation instead + * of once per candidate file. Staleness is bounded to that one operation; + * writes are still gated by the commit check (assertMutationCommittable). + */ + private readonly ownerByContext = new WeakMap(); + + /** + * Owner of the workspace scope for this context ("" when there is no + * workspace). Public so callers that key sidecar metadata for the same + * context (memoryOperations) bind to the exact owner the store resolved to. + */ + ownerWorkspaceIdFor(ctx: MemoryScopeContext): string { + if (ctx.workspaceId === "") return ""; + const cached = this.ownerByContext.get(ctx); + if (cached !== undefined) return cached; + const owner = this.resolveWorkspaceMemoryOwnerId(ctx.workspaceId); + this.ownerByContext.set(ctx, owner); + return owner; + } + /** Logical sidecar key, or null when the scope has no stable identity. */ private logicalKeyFor(ctx: MemoryScopeContext, scope: MemoryScope, relPath: string) { if (scope === "project" && ctx.projectPath === "") return null; return memoryLogicalKey(scope, relPath, { projectPath: ctx.projectPath, - workspaceId: ctx.workspaceId, + // Pins/usage stats follow the physical file, so a shared notebook has + // one ranking regardless of which tree member touched it. Only the + // workspace key embeds the id; skip the lookup for the other scopes. + workspaceId: scope === "workspace" ? this.ownerWorkspaceIdFor(ctx) : ctx.workspaceId, }); } + /** + * Consolidation's pin protection (pinned files are editable but never + * deleted/renamed; a directory counts when anything under it is pinned), + * evaluated INSIDE the mutation lock against the owner the command's store + * is bound to: logicalKeyFor and getStore share this command's owner + * resolution (ownerWorkspaceIdFor), so the key checked is the key of the + * file about to be removed. A guard run before the command against a + * separately resolved owner (the private-store fallback while config.json + * was unreadable) would check the wrong sidecar entries and let an + * owner-pinned note go. + */ + private async assertNotPinnedForRemoval( + ctx: MemoryScopeContext, + scope: MemoryScope, + relPath: string, + virtualPath: string + ): Promise { + const key = this.logicalKeyFor(ctx, scope, relPath); + if (key === null) return; + const subtreePrefix = `${key}/`; + for (const [entryKey, entry] of await this.metaService.getEntries()) { + if (entry.pinned !== true) continue; + if (entryKey === key || entryKey.startsWith(subtreePrefix)) { + throw new MemoryCommandError( + `${virtualPath} is pinned by the user (directly or via a pinned file inside it); pinned files may be edited but never deleted or renamed.` + ); + } + } + } + private async recordUsage( ctx: MemoryScopeContext, scope: MemoryScope, @@ -627,6 +809,13 @@ export class MemoryService extends EventEmitter { const key = this.logicalKeyFor(ctx, scope, relPath); if (key === null) return; await this.metaService.recordAccess(key, options); + if (scope === "workspace" && !options.write) { + // A read-side access (view, recall) re-ranks the shared hot set the + // whole task tree derives from the owner's sidecar entries, so it is + // published like a pin: the other live sessions of the tree drop + // their cached memory context. Writes publish with their mutation. + this.emitChange(ctx, scope, relPath, "agent"); + } } catch (error) { log.debug("[MemoryService] failed to record memory usage", { scope, relPath, error }); } @@ -717,15 +906,22 @@ export class MemoryService extends EventEmitter { ); } return new LocalMemoryStore( - workspaceMemoryStorePath(this.config.sessionsDir, ctx.workspaceId) + workspaceMemoryStorePath(this.config.sessionsDir, this.ownerWorkspaceIdFor(ctx)) ); } } } private async runCommand( + ctx: MemoryScopeContext, operation: () => Promise ): Promise { + // The per-context owner cache is scoped to ONE command: createMemoryTool + // reuses a context for a whole stream, and a cached owner would otherwise + // let a child keep reading its parent's notebook after the tree changed + // (owner removed by another backend — no local event) for as long as the + // stream lives. Re-resolving costs one memoized, stamp-validated lookup. + this.ownerByContext.delete(ctx); try { return await operation(); } catch (error) { @@ -769,7 +965,8 @@ export class MemoryService extends EventEmitter { * Append the invertible `refinement` row for one memory mutation (RLM r2). * * Rows land in the ACTING workspace's session journal even though memory - * files can be global/project-scoped: the journal is per-session, so + * files can be global/project-scoped — or, for a sub-agent's workspace + * scope, live in the OWNER's session dir: the journal is per-session, so * cross-workspace edits to a shared file are attributed to (and invertible * from) whichever workspace made them — the intended v1 scope. When the * context has no workspace, there is no session journal; skip (log-only). @@ -804,6 +1001,71 @@ export class MemoryService extends EventEmitter { }); } + /** + * Refuse to COMMIT a mutation whose caller was torn down (r59/r61). Checked + * INSIDE the target mutation lock immediately before the first durable + * write; a mutation that already committed always journals (mutation → row + * → ack) so rollback lineage stays intact. Two teardown signals: + * + * - The caller's abort signal (r59): consolidation/refine passes receive no + * hard tool cancellation — an execution wedged in pre-commit I/O (e.g. a + * named pipe under a memory root) is detached by the caller's bounded + * drain, and once the I/O unblocks after workspace teardown it would + * still write durable memory AND append its refinement journal row into + * the deleted session directory, recreating it. + * - The durable removal tombstone (r61): with multiple backends over one + * Xum root, the remover cannot abort a dream/harvest run in ANOTHER + * process — that run's signal stays live after removal. The tombstone is + * published under the same memory target locks this check runs inside + * (see workspaceRemoval.ts), so a foreign backend's mutation observes + * removal here at commit time and refuses instead of recreating the + * deleted session directory via its write or journal append. + * + * The owner the command's store was bound to is then compared with a fresh + * resolution: the per-context cache (ownerWorkspaceIdFor) may hold a + * self-fallback taken while config.json was missing or malformed, and if + * the file recovers before this command commits, the write would land in + * the child's private store although the tree is shared again. Refused as + * a recoverable error; the retried command resolves the owner anew. + */ + private async assertMutationCommittable( + ctx: MemoryScopeContext, + signal: AbortSignal | undefined, + virtualPath: string + ): Promise { + if (signal?.aborted === true) { + throw new MemoryCommandError( + `Mutation of ${virtualPath} was cancelled before commit (caller torn down)` + ); + } + if (ctx.workspaceId === "") return; + const boundOwner = this.ownerByContext.get(ctx); + if (boundOwner !== undefined) { + const currentOwner = this.resolveWorkspaceMemoryOwnerId(ctx.workspaceId); + if (currentOwner !== boundOwner) { + throw new MemoryCommandError( + `Ownership of the workspace notebook changed while mutating ${virtualPath} (now ${currentOwner}); retry the command` + ); + } + } + // The acting workspace AND, for the workspace scope, the store owner: a + // child's mutation waits on the owner's store lock while the owner is + // removed (tombstone published, session dir deleted), then resumes and + // would recreate the owner's directory. Global/project stores are not + // the owner's, so a removed owner does not refuse those. + const storeOwner = + parseMemoryPath(virtualPath).scope === "workspace" + ? this.ownerWorkspaceIdFor(ctx) + : undefined; + for (const workspaceId of new Set([ctx.workspaceId, storeOwner ?? ctx.workspaceId])) { + if (await isWorkspaceRemovalTombstoned(this.config.rootDir, workspaceId)) { + throw new MemoryCommandError( + `Workspace ${workspaceId} was removed; refusing to commit the mutation of ${virtualPath}` + ); + } + } + } + /** * Capture the restore payload for a delete (file or recursive directory) * BEFORE it is removed. Returns null when capture fails or the subtree @@ -922,12 +1184,47 @@ export class MemoryService extends EventEmitter { scope, path: toVirtualPath(scope, relPath), actor, - workspaceId: ctx.workspaceId, + // Owner, not actor: subscribers filter workspace-scope events by the + // store they display, and every tree member displays the owner's. + workspaceId: this.ownerWorkspaceIdFor(ctx), projectPath: ctx.projectPath, }; this.emit("change", event); } + /** + * Toggle a pin (Memory tab). Pins live in the sidecar, not the store, so + * nothing else emits a change: for the workspace scope the sidecar write + * happens under the store's mutation lock, so a lock timeout fails BEFORE + * anything is committed (no durable pin with a failed route), and the other + * tree members' tabs are told afterwards. Sidecar write failures surface as + * MemoryMetaWriteError. + */ + async setPinned(ctx: MemoryScopeContext, virtualPath: string, pinned: boolean): Promise { + const parsed = parseMemoryPath(virtualPath); + const scope = this.requireFilePath(parsed, virtualPath); + const key = this.logicalKeyFor(ctx, scope, parsed.relPath); + if (key === null) { + throw new MemoryCommandError( + "Project memory is unavailable: no project is associated with this session" + ); + } + if (scope === "workspace") { + const store = this.getStore(ctx, scope); + await withTargetMutationLock(this.config.rootDir, this.storeLockKey(store), async () => { + // Same commit guard as file mutations: `key` and `store` were bound to + // the owner the context resolved BEFORE the lock. If ownership moved + // meanwhile, the pin would land under a dead logical key and the + // route would still report success. Refuse instead. + await this.assertMutationCommittable(ctx, undefined, virtualPath); + await this.metaService.setPinned(key, pinned); + }); + } else { + await this.metaService.setPinned(key, pinned); + } + this.emitChange(ctx, scope, parsed.relPath, "user"); + } + /** * Announces that a project's memory was mutated outside this service. The settings-backup * restore writes memory files directly (under the shared memory mutation lock), and @@ -958,7 +1255,7 @@ export class MemoryService extends EventEmitter { virtualPath: string, options?: { offset?: number; limit?: number } ): Promise { - return this.runCommand(async () => { + return this.runCommand(ctx, async () => { const parsed = parseMemoryPath(virtualPath); if (parsed.scope === null) { // Virtual root: list every scope. @@ -1015,7 +1312,7 @@ export class MemoryService extends EventEmitter { toolCallId?: string, abortSignal?: AbortSignal ): Promise { - return this.runCommand(async () => { + return this.runCommand(ctx, async () => { const parsed = parseMemoryPath(virtualPath); const scope = this.requireFilePath(parsed, virtualPath); assertWithinFileSizeCap(fileText); @@ -1025,7 +1322,7 @@ export class MemoryService extends EventEmitter { // only INSIDE the lock and after the removal check (r62), so the // mkdir serializes with removal's locked deletion and cannot // recreate a removed session directory. - await assertMutationCommittable(this.config.rootDir, ctx, abortSignal, virtualPath); + await this.assertMutationCommittable(ctx, abortSignal, virtualPath); await store.ensureRoot(); const existing = await store.kind(parsed.relPath); if (existing !== null) { @@ -1039,7 +1336,7 @@ export class MemoryService extends EventEmitter { `The ${scope} memory scope is full (${MEMORY_MAX_FILES_PER_SCOPE} files); delete unused files first` ); } - await assertMutationCommittable(this.config.rootDir, ctx, abortSignal, virtualPath); + await this.assertMutationCommittable(ctx, abortSignal, virtualPath); await store.writeFile(parsed.relPath, fileText); // Row is written before the create is acknowledged (mutation → row → ack). await this.journalRefinement( @@ -1069,7 +1366,7 @@ export class MemoryService extends EventEmitter { toolCallId?: string, abortSignal?: AbortSignal ): Promise { - return this.runCommand(async () => { + return this.runCommand(ctx, async () => { const parsed = parseMemoryPath(virtualPath); const scope = this.requireFilePath(parsed, virtualPath); if (oldStr.length === 0) { @@ -1080,7 +1377,7 @@ export class MemoryService extends EventEmitter { const content = await this.readTextFileForEdit(store, parsed.relPath, virtualPath); const updated = computeStrReplaceUpdate(content, oldStr, newStr, virtualPath); assertWithinFileSizeCap(updated); - await assertMutationCommittable(this.config.rootDir, ctx, abortSignal, virtualPath); + await this.assertMutationCommittable(ctx, abortSignal, virtualPath); await store.writeFile(parsed.relPath, updated); // Row is written before the edit is acknowledged (mutation → row → ack). await this.journalRefinement( @@ -1111,7 +1408,7 @@ export class MemoryService extends EventEmitter { expectedFingerprint?: string, abortSignal?: AbortSignal ): Promise { - return this.runCommand(async () => { + return this.runCommand(ctx, async () => { const parsed = parseMemoryPath(virtualPath); const scope = this.requireFilePath(parsed, virtualPath); const store = await this.resolveStore(ctx, scope, parsed.relPath); @@ -1133,7 +1430,7 @@ export class MemoryService extends EventEmitter { const content = await this.readTextFileForEdit(store, parsed.relPath, virtualPath); const { updated, insertedLineCount } = computeInsertUpdate(content, insertLine, insertText); assertWithinFileSizeCap(updated); - await assertMutationCommittable(this.config.rootDir, ctx, abortSignal, virtualPath); + await this.assertMutationCommittable(ctx, abortSignal, virtualPath); await store.writeFile(parsed.relPath, updated); // Row is written before the edit is acknowledged (mutation → row → ack). await this.journalRefinement( @@ -1185,7 +1482,7 @@ export class MemoryService extends EventEmitter { toolCallId?: string, abortSignal?: AbortSignal ): Promise { - return this.runCommand(async () => { + return this.runCommand(ctx, async () => { const parsed = parseMemoryPath(virtualPath); const scope = this.requireFilePath(parsed, virtualPath); if (mutation.command === "str_replace" && mutation.oldStr.length === 0) { @@ -1193,7 +1490,7 @@ export class MemoryService extends EventEmitter { } const store = await this.resolveStore(ctx, scope, parsed.relPath); return withTargetMutationLock(this.config.rootDir, this.storeLockKey(store), async () => { - await assertMutationCommittable(this.config.rootDir, ctx, abortSignal, virtualPath); + await this.assertMutationCommittable(ctx, abortSignal, virtualPath); await store.ensureRoot(); const kind = await store.kind(parsed.relPath); if (kind === "dir") { @@ -1228,7 +1525,7 @@ export class MemoryService extends EventEmitter { mutation.insertText ).updated; assertWithinFileSizeCap(updated, maxFileBytes); - await assertMutationCommittable(this.config.rootDir, ctx, abortSignal, virtualPath); + await this.assertMutationCommittable(ctx, abortSignal, virtualPath); await store.writeFile(parsed.relPath, updated); const physicalPath = store.physicalPath(parsed.relPath); // Row is written before the write is acknowledged (mutation → row → ack). @@ -1261,7 +1558,7 @@ export class MemoryService extends EventEmitter { | { command: "delete"; path: string } | { command: "rename"; path: string; new_path: string } ): Promise<{ ok: true } | { ok: false; error: string }> { - const result = await this.runCommand(async () => { + const result = await this.runCommand(ctx, async () => { const parsed = parseMemoryPath(command.path); const scope = this.requireFilePath(parsed, command.path); switch (command.command) { @@ -1374,9 +1671,10 @@ export class MemoryService extends EventEmitter { actor: MemoryActor, toolCallId?: string, expectedFingerprint?: string, - abortSignal?: AbortSignal + abortSignal?: AbortSignal, + options?: { rejectPinned?: boolean } ): Promise { - return this.runCommand(async () => { + return this.runCommand(ctx, async () => { const parsed = parseMemoryPath(virtualPath); const scope = this.requireFilePath(parsed, virtualPath); const store = await this.resolveStore(ctx, scope, parsed.relPath); @@ -1385,6 +1683,9 @@ export class MemoryService extends EventEmitter { if (kind === null) { throw new MemoryCommandError(`No memory file or directory at ${virtualPath}`); } + if (options?.rejectPinned === true) { + await this.assertNotPinnedForRemoval(ctx, scope, parsed.relPath, virtualPath); + } // r55: staged refine deletes were approved against the target's // staging-time state — a target edited between staging and apply // must refuse rather than silently destroying the newer contents. @@ -1401,7 +1702,7 @@ export class MemoryService extends EventEmitter { // Prior contents must be captured before removal; the row itself is // written after the mutation succeeds and before it is acknowledged. const inverse = await this.captureDeleteInverse(store, parsed.relPath, kind); - await assertMutationCommittable(this.config.rootDir, ctx, abortSignal, virtualPath); + await this.assertMutationCommittable(ctx, abortSignal, virtualPath); await store.remove(parsed.relPath); if (inverse !== null) { await this.journalRefinement( @@ -1428,9 +1729,10 @@ export class MemoryService extends EventEmitter { newVirtualPath: string, actor: MemoryActor, toolCallId?: string, - abortSignal?: AbortSignal + abortSignal?: AbortSignal, + options?: { rejectPinned?: boolean } ): Promise { - return this.runCommand(async () => { + return this.runCommand(ctx, async () => { const oldParsed = parseMemoryPath(oldVirtualPath); const newParsed = parseMemoryPath(newVirtualPath); const scope = this.requireFilePath(oldParsed, oldVirtualPath); @@ -1448,6 +1750,9 @@ export class MemoryService extends EventEmitter { if (oldKind === null) { throw new MemoryCommandError(`No memory file or directory at ${oldVirtualPath}`); } + if (options?.rejectPinned === true) { + await this.assertNotPinnedForRemoval(ctx, scope, oldParsed.relPath, oldVirtualPath); + } // Pre-flight (mirrored in validateMutation): store.rename would mkdir // the destination parent INSIDE the source before the filesystem // rejects the move — refuse cleanly instead of polluting the source. @@ -1463,7 +1768,7 @@ export class MemoryService extends EventEmitter { if (newKind !== null) { throw new MemoryCommandError(`Destination ${newVirtualPath} already exists`); } - await assertMutationCommittable(this.config.rootDir, ctx, abortSignal, oldVirtualPath); + await this.assertMutationCommittable(ctx, abortSignal, oldVirtualPath); await store.rename(oldParsed.relPath, newParsed.relPath); // Row is written before the rename is acknowledged (mutation → row → ack). await this.journalRefinement( @@ -1587,7 +1892,7 @@ export class MemoryService extends EventEmitter { async () => { // UI save can create new files: materialize the scope root on // first use — in-lock, after the removal check (r62; see create). - await assertMutationCommittable(this.config.rootDir, ctx, abortSignal, virtualPath); + await this.assertMutationCommittable(ctx, abortSignal, virtualPath); await store.ensureRoot(); const kind = await store.kind(parsed.relPath); if (kind === "dir") { @@ -1614,7 +1919,7 @@ export class MemoryService extends EventEmitter { ); } } - await assertMutationCommittable(this.config.rootDir, ctx, abortSignal, virtualPath); + await this.assertMutationCommittable(ctx, abortSignal, virtualPath); await store.writeFile(parsed.relPath, content); await this.recordUsage(ctx, scope, parsed.relPath, { write: true }); this.emitChange(ctx, scope, parsed.relPath, actor); @@ -1807,44 +2112,6 @@ function sha256Hex(content: string): string { return createHash("sha256").update(content, "utf-8").digest("hex"); } -/** - * Refuse to COMMIT a mutation whose caller was torn down (r59/r61). Checked - * INSIDE the target mutation lock immediately before the first durable - * write; a mutation that already committed always journals (mutation → row - * → ack) so rollback lineage stays intact. Two teardown signals: - * - * - The caller's abort signal (r59): consolidation/refine passes receive no - * hard tool cancellation — an execution wedged in pre-commit I/O (e.g. a - * named pipe under a memory root) is detached by the caller's bounded - * drain, and once the I/O unblocks after workspace teardown it would - * still write durable memory AND append its refinement journal row into - * the deleted session directory, recreating it. - * - The durable removal tombstone (r61): with multiple backends over one - * Xum root, the remover cannot abort a dream/harvest run in ANOTHER - * process — that run's signal stays live after removal. The tombstone is - * published under the same memory target locks this check runs inside - * (see workspaceRemoval.ts), so a foreign backend's mutation observes - * removal here at commit time and refuses instead of recreating the - * deleted session directory via its write or journal append. - */ -async function assertMutationCommittable( - rootDir: string, - ctx: MemoryScopeContext, - signal: AbortSignal | undefined, - virtualPath: string -): Promise { - if (signal?.aborted === true) { - throw new MemoryCommandError( - `Mutation of ${virtualPath} was cancelled before commit (caller torn down)` - ); - } - if (ctx.workspaceId !== "" && (await isWorkspaceRemovalTombstoned(rootDir, ctx.workspaceId))) { - throw new MemoryCommandError( - `Workspace ${ctx.workspaceId} was removed; refusing to commit the mutation of ${virtualPath}` - ); - } -} - /** * Deterministic, lenient hash of a physical subtree for delete-target change * detection (r55/r58, see MemoryService.fingerprintMutationTarget). Sorted walk; diff --git a/src/node/services/memoryWorkspaceOwner.test.ts b/src/node/services/memoryWorkspaceOwner.test.ts new file mode 100644 index 0000000000..f11101fbec --- /dev/null +++ b/src/node/services/memoryWorkspaceOwner.test.ts @@ -0,0 +1,70 @@ +import { describe, expect, it } from "bun:test"; +import type { Config } from "@/node/config"; +import { + resolveWorkspaceMemoryOwnerId, + workspaceMemoryOwnerResolver, +} from "./memoryWorkspaceOwner"; + +type ProjectsConfig = ReturnType; + +function topology(workspaces: Array<{ id: string; parentWorkspaceId?: string }>): ProjectsConfig { + return { + projects: new Map([ + ["/tmp/project", { workspaces: workspaces.map((ws) => ({ path: `/tmp/${ws.id}`, ...ws })) }], + ]), + } as unknown as ProjectsConfig; +} + +describe("resolveWorkspaceMemoryOwnerId", () => { + it("resolves the task-tree root; unknown and parentless ids resolve to themselves", () => { + const cfg = topology([ + { id: "ws-owner" }, + { id: "ws-child", parentWorkspaceId: "ws-owner" }, + { id: "ws-grandchild", parentWorkspaceId: "ws-child" }, + { id: "ws-solo" }, + ]); + expect(resolveWorkspaceMemoryOwnerId(cfg, "ws-owner")).toBe("ws-owner"); + expect(resolveWorkspaceMemoryOwnerId(cfg, "ws-child")).toBe("ws-owner"); + expect(resolveWorkspaceMemoryOwnerId(cfg, "ws-grandchild")).toBe("ws-owner"); + expect(resolveWorkspaceMemoryOwnerId(cfg, "ws-solo")).toBe("ws-solo"); + expect(resolveWorkspaceMemoryOwnerId(cfg, "ws-unregistered")).toBe("ws-unregistered"); + }); + + it("falls back to the acting workspace on a dangling parent, a cycle, or an over-deep chain", () => { + // Dangling: the recorded parent is not registered (removed, or never was). + const dangling = topology([ + { id: "ws-orphan", parentWorkspaceId: "ws-gone" }, + { id: "ws-deep", parentWorkspaceId: "ws-orphan" }, + ]); + expect(resolveWorkspaceMemoryOwnerId(dangling, "ws-orphan")).toBe("ws-orphan"); + // A chain that dangles above the caller resolves to the CALLER, not to the + // last registered ancestor: the fallback keeps the private store usable. + expect(resolveWorkspaceMemoryOwnerId(dangling, "ws-deep")).toBe("ws-deep"); + + const cycle = topology([ + { id: "ws-a", parentWorkspaceId: "ws-b" }, + { id: "ws-b", parentWorkspaceId: "ws-a" }, + { id: "ws-c", parentWorkspaceId: "ws-a" }, + ]); + expect(resolveWorkspaceMemoryOwnerId(cycle, "ws-a")).toBe("ws-a"); + expect(resolveWorkspaceMemoryOwnerId(cycle, "ws-c")).toBe("ws-c"); + + const deep = topology( + Array.from({ length: 40 }, (_, i) => ({ + id: `ws-${i}`, + ...(i === 0 ? {} : { parentWorkspaceId: `ws-${i - 1}` }), + })) + ); + expect(resolveWorkspaceMemoryOwnerId(deep, "ws-20")).toBe("ws-0"); + expect(resolveWorkspaceMemoryOwnerId(deep, "ws-39")).toBe("ws-39"); + }); + + it("indexes a snapshot once and reuses the resolver for it", () => { + const cfg = topology([{ id: "ws-owner" }, { id: "ws-child", parentWorkspaceId: "ws-owner" }]); + const resolve = workspaceMemoryOwnerResolver(cfg); + expect(workspaceMemoryOwnerResolver(cfg)).toBe(resolve); + expect(resolve("ws-child")).toBe("ws-owner"); + // A different snapshot object gets its own index. + expect(workspaceMemoryOwnerResolver(topology([{ id: "ws-owner" }]))).not.toBe(resolve); + }); +}); diff --git a/src/node/services/memoryWorkspaceOwner.ts b/src/node/services/memoryWorkspaceOwner.ts new file mode 100644 index 0000000000..0092cca81c --- /dev/null +++ b/src/node/services/memoryWorkspaceOwner.ts @@ -0,0 +1,82 @@ +import assert from "@/common/utils/assert"; +import type { Config, Workspace as WorkspaceConfigEntry } from "@/node/config"; +import { log } from "@/node/services/log"; + +type ProjectsConfig = ReturnType; + +/** + * The workspace whose /memory backs `/memories/workspace` for + * `workspaceId`: the root of its parentWorkspaceId chain. Sub-agents (and + * nested sub-agents) thereby share ONE notebook with the workspace that + * spawned the task tree, while their transcripts/session artifacts stay + * separate. Full `kind: "workspace"` tasks and forks carry no + * parentWorkspaceId and own their notes. + * + * Unknown IDs, dangling parents, cycles, and depth overflow resolve to the ID + * itself so a misconfigured tree degrades to per-workspace behavior instead + * of failing every memory command. Callers that need "is this a shared + * child?" must compare the result to the input rather than test + * parentWorkspaceId, so those fallbacks keep their private store usable. + * + * Pure over one config snapshot (indexed once per snapshot, see + * workspaceMemoryOwnerResolver); MemoryService memoizes it. + */ +export function resolveWorkspaceMemoryOwnerId(cfg: ProjectsConfig, workspaceId: string): string { + return workspaceMemoryOwnerResolver(cfg)(workspaceId); +} + +/** + * Per-snapshot resolvers keyed by the config object: bulk passes (memo + * revalidation on every local config edit, launch sweep, change diffing) + * resolve many workspaces against one snapshot, and a linear + * findWorkspaceEntry per chain hop would make them O(n²) on the main process. + * The index is built once per snapshot and the snapshot is never mutated + * after load, so a WeakMap keyed by identity is safe. + */ +const resolversBySnapshot = new WeakMap string>(); + +export function workspaceMemoryOwnerResolver(cfg: ProjectsConfig): (workspaceId: string) => string { + const cached = resolversBySnapshot.get(cfg); + if (cached !== undefined) return cached; + const byId = new Map(); + for (const project of cfg.projects.values()) { + for (const workspace of project.workspaces) { + if (workspace.id !== undefined) byId.set(workspace.id, workspace); + } + } + const resolver = (workspaceId: string): string => { + assert(workspaceId.length > 0, "resolveWorkspaceMemoryOwnerId requires a workspaceId"); + let current = workspaceId; + const visited = new Set(); + for (let depth = 0; depth < 32; depth++) { + if (visited.has(current)) { + log.warn("[memory] parentWorkspaceId cycle; using acting workspace as memory owner", { + workspaceId, + }); + return workspaceId; + } + visited.add(current); + const entry = byId.get(current); + if (entry === undefined) { + // Only the chain root may be unknown without invalidating the walk: an + // unregistered starting workspace simply resolves to itself. + if (current !== workspaceId) { + log.warn("[memory] parentWorkspaceId points at an unknown workspace", { + workspaceId, + parentWorkspaceId: current, + }); + } + return workspaceId; + } + const parentWorkspaceId = entry.parentWorkspaceId; + if (parentWorkspaceId === undefined || parentWorkspaceId === "") return current; + current = parentWorkspaceId; + } + log.warn("[memory] parentWorkspaceId chain too deep; using acting workspace as memory owner", { + workspaceId, + }); + return workspaceId; + }; + resolversBySnapshot.set(cfg, resolver); + return resolver; +} diff --git a/src/node/services/tools/memory.ts b/src/node/services/tools/memory.ts index 7fd482d26c..6f673590c0 100644 --- a/src/node/services/tools/memory.ts +++ b/src/node/services/tools/memory.ts @@ -257,6 +257,13 @@ export async function executeMemoryCommand( * I/O unblocks. Ignored by reads. */ abortSignal?: AbortSignal; + /** + * Consolidation's pin protection (pinned files are editable but never + * deleted/renamed), enforced by MemoryService INSIDE its target mutation + * lock against the owner the command's store is bound to — see + * MemoryService.assertNotPinnedForRemoval. Ignored by other commands. + */ + rejectPinned?: boolean; } ): Promise { try { @@ -336,7 +343,8 @@ export async function executeMemoryCommand( "agent", toolCallId, options?.expectedTargetFingerprint, - options?.abortSignal + options?.abortSignal, + { rejectPinned: options?.rejectPinned } )) ); } @@ -355,7 +363,8 @@ export async function executeMemoryCommand( input.new_path, "agent", toolCallId, - options?.abortSignal + options?.abortSignal, + { rejectPinned: options?.rejectPinned } )) ); } diff --git a/src/node/services/workspaceRemoval.test.ts b/src/node/services/workspaceRemoval.test.ts index 970ccea6d1..75abc00ae0 100644 --- a/src/node/services/workspaceRemoval.test.ts +++ b/src/node/services/workspaceRemoval.test.ts @@ -80,6 +80,62 @@ describe("workspaceRemoval", () => { expect((JSON.parse(raw) as { workspaceId: string }).workspaceId).toBe(workspaceId); }); + test("sub-agent removal also waits for a writer holding the OWNER's memory store lock", async () => { + using tmp = new DisposableTempDir("workspace-removal-test"); + const rootDir = path.join(tmp.path, "xum-home"); + const ownerSessionDir = path.join(rootDir, "sessions", "ws-owner"); + const childId = "ws-child"; + const childSessionDir = path.join(rootDir, "sessions", childId); + await fsPromises.mkdir(path.join(ownerSessionDir, "memory"), { recursive: true }); + await fsPromises.mkdir(childSessionDir, { recursive: true }); + + // A sub-agent's admitted workspace-memory write holds the OWNER store's + // lock (that is where the shared notebook lives), not the child's. + let releaseWriter!: () => void; + const writerGate = new Promise((resolve) => (releaseWriter = resolve)); + let writerEntered!: () => void; + const entered = new Promise((resolve) => (writerEntered = resolve)); + const writer = withTargetMutationLock( + rootDir, + path.join(ownerSessionDir, "memory"), + async () => { + writerEntered(); + await writerGate; + } + ); + await entered; + + const removal = removeSessionDirUnderMemoryLocks({ + rootDir, + sessionDir: childSessionDir, + workspaceId: childId, + attemptId: "test-attempt", + sharedWorkspaceMemorySessionDir: ownerSessionDir, + }); + await new Promise((resolve) => setTimeout(resolve, 100)); + // Without the owner lock the child tombstone would already be published + // here, between the writer's commit check and its durable write. + expect(await isWorkspaceRemovalTombstoned(rootDir, childId)).toBe(false); + + releaseWriter(); + await writer; + await removal; + expect(await isWorkspaceRemovalTombstoned(rootDir, childId)).toBe(true); + // Only the child's session dir is deleted; the owner keeps its notebook. + expect( + await fsPromises.access(childSessionDir).then( + () => true, + () => false + ) + ).toBe(false); + expect( + await fsPromises.access(path.join(ownerSessionDir, "memory")).then( + () => true, + () => false + ) + ).toBe(true); + }); + test("waits on the refine lock BEFORE taking the teardown target locks (r67)", async () => { using tmp = new DisposableTempDir("workspace-removal-test"); const rootDir = path.join(tmp.path, "xum-home"); diff --git a/src/node/services/workspaceRemoval.ts b/src/node/services/workspaceRemoval.ts index 154c731162..1b5b06a1e5 100644 --- a/src/node/services/workspaceRemoval.ts +++ b/src/node/services/workspaceRemoval.ts @@ -142,6 +142,16 @@ export async function removeSessionDirUnderMemoryLocks(args: { * succeeding or still-active) attempt relies on. */ attemptId: string; + /** + * Session dir of the workspace whose `memory/` this workspace's + * `/memories/workspace` resolves to when that is ANOTHER workspace (a + * sub-agent sharing its task tree's notebook, memoryWorkspaceOwner.ts). + * Its store lock is held too: a child mutation is admitted under the + * OWNER's store key while journaling into this session dir, so a removal + * that took only this session's keys could publish the tombstone between + * the mutation's tombstone check and its commit. + */ + sharedWorkspaceMemorySessionDir?: string; }): Promise { assert(args.sessionDir.length > 0, "removeSessionDirUnderMemoryLocks requires a session dir"); // Crash clearly on a malformed config (test stubs, future refactors): an @@ -159,6 +169,15 @@ export async function removeSessionDirUnderMemoryLocks(args: { path.join(args.sessionDir, "memory") ); const sharedMemoryKey = memoryMutationLockKey(args.rootDir, path.join(args.rootDir, "memory")); + const ownerMemoryKeys = + args.sharedWorkspaceMemorySessionDir === undefined + ? [] + : [ + memoryMutationLockKey( + args.rootDir, + path.join(args.sharedWorkspaceMemorySessionDir, "memory") + ), + ]; // The session dir itself is a third target key (r63): session-scoped // sidecar writers (headless usage) serialize their tombstone check + // commit against this same key, closing their check→write window. @@ -200,7 +219,7 @@ export async function removeSessionDirUnderMemoryLocks(args: { }); await withTargetMutationLocks( args.rootDir, - [sessionDirKey, workspaceMemoryKey, sharedMemoryKey], + [sessionDirKey, workspaceMemoryKey, sharedMemoryKey, ...ownerMemoryKeys], async () => { // History append serialization (r63): a foreign backend's in-flight // stream can be mid-append under the history write lock; acquiring diff --git a/src/node/services/workspaceService.ts b/src/node/services/workspaceService.ts index e06098f019..4baa0e055b 100644 --- a/src/node/services/workspaceService.ts +++ b/src/node/services/workspaceService.ts @@ -130,6 +130,7 @@ import { deriveSideChannelModelCandidates, startAbandonedBranchSummaryInBackground, } from "@/node/services/branchSummary"; +import { resolveWorkspaceMemoryOwnerId } from "@/node/services/memoryWorkspaceOwner"; import { healRemovalTombstonesForRegisteredWorkspaces, removeSessionDirUnderMemoryLocks, @@ -4200,6 +4201,21 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { for (const session of this.sessions.values()) session.beginShutdown(); } + /** + * Sub-agents share their task-tree owner's /memories/workspace store, so a + * workspace-scope write by one tree member stales the cached memory context + * of every live session in that tree, not just the acting one (which already + * clears its own cache on tool-call-end). `isAffected` decides membership. + */ + invalidateMemoryContextWhere(isAffected: (workspaceId: string) => boolean): void { + // Startup-recovery sessions are live too and may be promoted with their cache. + for (const registry of [this.sessions, this.transientStartupRecoverySessions]) { + for (const [workspaceId, session] of registry) { + if (isAffected(workspaceId)) session.invalidateMemoryContext(); + } + } + } + /** Transfer destructive cleanup out of a callback that still owns a session lease. */ deferWorkspaceCleanup(run: () => Promise): void { this.trackWorkspaceCleanup(run).catch((error: unknown) => @@ -6315,11 +6331,23 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { // directory. Fail-closed on a wedged writer: the catch below keeps // the directory as a recoverable orphan instead of deleting it out // from under a live commit. + // A sub-agent's workspace notes live in its task-tree owner's store: + // hold that store's lock too, so a child mutation admitted under the + // owner key cannot commit after this tombstone. The workspace is + // still registered here, so its parent chain resolves. + const memoryOwnerId = resolveWorkspaceMemoryOwnerId( + this.config.loadConfigOrDefault(), + workspaceId + ); await removeSessionDirUnderMemoryLocks({ rootDir: this.config.rootDir, sessionDir, workspaceId, attemptId: removalAttemptId, + sharedWorkspaceMemorySessionDir: + memoryOwnerId === workspaceId + ? undefined + : path.join(this.config.sessionsDir, memoryOwnerId), }); } catch (error) { // r63: without a durable tombstone the retained orphan stays