diff --git a/src/node/services/di/layers/core.ts b/src/node/services/di/layers/core.ts index 225ff77f86..4ed95035a5 100644 --- a/src/node/services/di/layers/core.ts +++ b/src/node/services/di/layers/core.ts @@ -573,6 +573,7 @@ export const CoreWiringLive: Layer.Layer< workspaceService.emitWorkflowRunActivity(event); turnRequestBuilderBindings.workflowResultContinuationSender = workspaceService; workspaceService.setMemoryConsolidationService(memoryConsolidationService); + workspaceService.setSharedWorkspaceMemoryStore(memoryService); // 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) => { diff --git a/src/node/services/memoryLegacyAdoption.ts b/src/node/services/memoryLegacyAdoption.ts new file mode 100644 index 0000000000..e1342e646d --- /dev/null +++ b/src/node/services/memoryLegacyAdoption.ts @@ -0,0 +1,174 @@ +/** + * Legacy-notebook adoption manifest: the durable record of which files of a + * sub-agent's PRE-SHARING private notebook (`/memory`, written by + * builds that kept `/memories/workspace` per workspace) were folded into the + * task-tree owner's shared store, and where each landed + * (MemoryService.adoptLegacyPrivateStore). + */ +import * as fsPromises from "node:fs/promises"; +import * as path from "node:path"; + +/** + * File in the sub-agent's SESSION dir (beside its legacy `memory` dir, never + * inside it) recording, per relPath, the sha256 of the content already copied + * into the shared store (adoptLegacyPrivateStore). Outside the legacy root on + * purpose: everything under `/memory` is the model-writable + * `/memories/workspace` namespace of a downgraded build (the path grammar + * admits dotfiles), and this manifest's `created`/`target`/hash fields are + * trusted as provenance. The session dir itself is not addressable through + * any memory path. + */ +export const LEGACY_ADOPTION_MANIFEST_FILE_NAME = "memory-adoption-manifest.json"; + +export function legacyAdoptionManifestPath(childSessionDir: string): string { + return path.join(childSessionDir, LEGACY_ADOPTION_MANIFEST_FILE_NAME); +} + +/** + * One adopted legacy file: content hash, child sidecar fingerprint, owner-store + * relPath, and whether the adoption CREATED that owner file (provenance: a + * pre-existing identical owner note is the owner's own). `pending`: written + * BEFORE the copy lands (provenance must not depend on the copy's existence: a + * retry finding the bytes already at the target could not tell an interrupted + * adoption from an owner note); cleared once the sidecar fold completed. + * + * Every field beyond the three strings is optional and unknown fields are + * ignored on read, so later builds can extend the record (downgrade-time + * reconciliation of edited/deleted sources) without invalidating manifests + * written by this one. + */ +export interface LegacyAdoptionRecord { + content: string; + sidecar: string; + target: string; + created?: boolean; + pending?: boolean; + /** + * Identity of the owner file this adoption wrote (`ino:size:mtimeNs` right + * after the write): the copy is THIS generation of the file, not merely a + * file holding the adopted bytes — an owner who deleted and recreated (or + * edited and restored) the note to identical bytes owns the new file. + * Absent (write before stamping, or the stamp could not be taken): the copy + * is never treated as this adoption's. + */ + targetStamp?: string; +} + +/** + * Parse one manifest record. Lifecycle flags are raw JSON: a value that is + * neither absent nor boolean fails CLOSED — `pending` reads as set (the pass + * is redone), `created` as unset (no destructive provenance) — so a corrupted + * flag can never make an interrupted pass look settled. + */ +function parseLegacyAdoptionRecord(value: unknown): LegacyAdoptionRecord | null { + if (typeof value !== "object" || value === null) return null; + const record = value as Record; + if ( + typeof record.content !== "string" || + typeof record.sidecar !== "string" || + typeof record.target !== "string" + ) { + return null; + } + if (record.targetStamp !== undefined && typeof record.targetStamp !== "string") return null; + const flag = (raw: unknown, malformed: boolean): boolean | undefined => + raw === undefined ? undefined : typeof raw === "boolean" ? raw : malformed; + return { + content: record.content, + sidecar: record.sidecar, + target: record.target, + created: flag(record.created, false), + pending: flag(record.pending, true), + targetStamp: record.targetStamp, + }; +} + +/** + * A manifest that exists and could be read but does not parse as a record + * map. Distinguished from an UNREADABLE file (EACCES, EIO — the plain fs + * error) so strict callers can quarantine the former (its bytes are the + * file's state) while still refusing on the latter. + */ +export class LegacyAdoptionManifestMalformedError extends Error { + constructor(manifestPath: string, detail: string) { + super(`the legacy adoption manifest at ${manifestPath} is malformed (${detail})`); + this.name = "LegacyAdoptionManifestMalformedError"; + } +} + +/** + * Read of the adoption manifest. A MISSING file reads as "nothing adopted" + * for every caller. Tolerant callers also read an unreadable (EACCES, EIO) + * or malformed file — bad JSON, a non-object, a record missing its string + * fields — as empty (self-healing: the next pass rewrites it). `strict` + * callers throw on all of those: the adoption pass and the removal handover + * decide what may be considered handed over on the manifest's authority, and + * an empty substitute would drop provenance. A Map, not a plain object: a + * legacy note may legitimately be named `__proto__` (any store-valid + * relPath), and assigning that key on an ordinary object hits the prototype + * setter instead of creating an entry the serialization would carry — the + * note would then be re-adopted on every access. JSON.parse and + * Object.fromEntries create own properties, so the round-trip is exact. + */ +export async function readLegacyAdoptionManifest( + manifestPath: string, + options?: { strict?: boolean } +): Promise> { + let raw: string; + try { + raw = await fsPromises.readFile(manifestPath, "utf-8"); + } catch (error) { + const code = (error as NodeJS.ErrnoException | null)?.code; + if (options?.strict === true && code !== "ENOENT" && code !== "ENOTDIR") throw error; + return new Map(); + } + const malformed = (detail: string): Map => { + if (options?.strict === true) { + throw new LegacyAdoptionManifestMalformedError(manifestPath, detail); + } + return new Map(); + }; + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch { + return malformed("not JSON"); + } + if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) { + return malformed("not an object"); + } + const entries: Array<[string, LegacyAdoptionRecord]> = []; + for (const [relPath, value] of Object.entries(parsed)) { + const record = parseLegacyAdoptionRecord(value); + if (record === null) return malformed(`record '${relPath}'`); + entries.push([relPath, record]); + } + return new Map(entries); +} + +/** + * The file identity a LegacyAdoptionRecord.targetStamp records, or why there + * is none: "absent" only when the stat PROVES the path is gone (ENOENT / + * ENOTDIR); any other failure (EACCES, EIO) is "unreadable" — it says + * nothing about the path, so callers deciding on absence must refuse. + */ +export async function adoptionTargetPresence( + absPath: string +): Promise<{ stamp: string } | "absent" | "unreadable"> { + try { + const stat = await fsPromises.lstat(absPath, { bigint: true }); + return { stamp: `${stat.ino}:${stat.size}:${stat.mtimeNs}` }; + } catch (error) { + const code = (error as NodeJS.ErrnoException | null)?.code; + return code === "ENOENT" || code === "ENOTDIR" ? "absent" : "unreadable"; + } +} + +/** + * The file identity a LegacyAdoptionRecord.targetStamp records; null when the + * file cannot be stat'ed (the record then carries no stamp: preserved). + */ +export async function adoptionTargetStamp(absPath: string): Promise { + const presence = await adoptionTargetPresence(absPath); + return typeof presence === "string" ? null : presence.stamp; +} diff --git a/src/node/services/memoryMeta.test.ts b/src/node/services/memoryMeta.test.ts index b39581fb4b..67929e71ba 100644 --- a/src/node/services/memoryMeta.test.ts +++ b/src/node/services/memoryMeta.test.ts @@ -1,10 +1,11 @@ -import { describe, it, expect } from "bun:test"; +import { describe, it, expect, spyOn } from "bun:test"; import { Effect } from "effect"; import * as fsPromises from "node:fs/promises"; import * as path from "node:path"; -import { MemoryMetaService, memoryLogicalKey } from "./memoryMeta"; +import { MemoryMetaService, MemoryMetaWriteError, memoryLogicalKey } from "./memoryMeta"; import { TestTempDir } from "./tools/testHelpers"; +import { getErrorMessage } from "@/common/utils/errors"; describe("memoryLogicalKey", () => { it("keys each scope by its stable identity, never the physical path", () => { @@ -249,4 +250,102 @@ describe("MemoryMetaService", () => { } }); }); + it("does not cache an empty view taken while the sidecar was unreadable", async () => { + using tempDir = new TestTempDir("test-memory-meta"); + const service = new MemoryMetaService(tempDir.path); + await service.setPinned("global:prefs.md", true); + // Transient read failure (EACCES interval): this read heals to empty, but + // the next one must retry the file — not serve the empty view and then + // write it back over the real pins. + const reader = spyOn(fsPromises, "readFile").mockImplementationOnce((() => + Promise.reject(Object.assign(new Error("EACCES"), { code: "EACCES" }))) as never); + const reloaded = new MemoryMetaService(tempDir.path); + expect(await reloaded.getPinnedKeys()).toEqual(new Set()); + reader.mockRestore(); + expect(await reloaded.getPinnedKeys()).toEqual(new Set(["global:prefs.md"])); + await reloaded.setPinned("workspace:ws-1:scratch.md", true); + expect(await new MemoryMetaService(tempDir.path).getPinnedKeys()).toEqual( + new Set(["global:prefs.md", "workspace:ws-1:scratch.md"]) + ); + }); + + it("refuses a mutation whose read of the sidecar failed instead of overwriting it", async () => { + using tempDir = new TestTempDir("test-memory-meta"); + await new MemoryMetaService(tempDir.path).setPinned("global:prefs.md", true); + // The mutating call itself hits the transient failure: its healed empty + // view must not become the file, or every existing pin is erased. + const reader = spyOn(fsPromises, "readFile").mockImplementationOnce((() => + Promise.reject(Object.assign(new Error("EACCES"), { code: "EACCES" }))) as never); + const fresh = new MemoryMetaService(tempDir.path); + try { + const failure = await fresh.setPinned("workspace:ws-1:scratch.md", true).then( + () => null, + (error: unknown) => error + ); + expect(failure).toBeInstanceOf(MemoryMetaWriteError); + expect((failure as MemoryMetaWriteError).reason).toContain("could not be read"); + } finally { + reader.mockRestore(); + } + expect(await new MemoryMetaService(tempDir.path).getPinnedKeys()).toEqual( + new Set(["global:prefs.md"]) + ); + // Once readable again the same instance mutates normally. + await fresh.setPinned("workspace:ws-1:scratch.md", true); + expect(await new MemoryMetaService(tempDir.path).getPinnedKeys()).toEqual( + new Set(["global:prefs.md", "workspace:ws-1:scratch.md"]) + ); + // getEntriesOrThrow refuses the healed substitute a plain read serves. + const strict = new MemoryMetaService(tempDir.path); + const strictReader = spyOn(fsPromises, "readFile").mockImplementationOnce((() => + Promise.reject(Object.assign(new Error("EACCES"), { code: "EACCES" }))) as never); + try { + const failure = await strict.getEntriesOrThrow().then( + () => null, + (error: unknown) => error + ); + expect(getErrorMessage(failure)).toContain("could not be read"); + } finally { + strictReader.mockRestore(); + } + expect((await strict.getEntriesOrThrow()).has("global:prefs.md")).toBe(true); + }); + + it("mergeKeys folds a subtree into a second key, keeping the source", async () => { + using tempDir = new TestTempDir("test-memory-meta"); + const service = new MemoryMetaService(tempDir.path); + // Child-keyed entries under one directory: one with no owner counterpart, + // one whose owner entry already has larger counters and its own pin. + await service.setPinned("workspace:ws-child:dir/only.md", true); + await service.recordAccess("workspace:ws-child:dir/both.md", { write: true }); + await service.setPinned("workspace:ws-child:dir/both.md", true); + for (let i = 0; i < 3; i++) { + await service.recordAccess("workspace:ws-owner:dir/both.md", { write: false }); + } + // A sibling whose key merely starts with the same characters is not in + // the subtree (segment-aware matching). + await service.setPinned("workspace:ws-child:dir-2/x.md", true); + await service.mergeKeys("workspace:ws-child:dir", "workspace:ws-owner:dir", { + pinned: "target", + }); + let entries = await service.getEntries(); + // Missing target: copied. Existing target: larger counters, its own pin. + expect(entries.get("workspace:ws-owner:dir/only.md")?.pinned).toBe(true); + expect(entries.get("workspace:ws-owner:dir/both.md")?.pinned).toBe(false); + expect(entries.get("workspace:ws-owner:dir/both.md")?.accessCount).toBe(3); + expect(entries.get("workspace:ws-owner:dir/both.md")?.lastWriteAt).not.toBeNull(); + expect(entries.has("workspace:ws-owner:dir-2/x.md")).toBe(false); + // The source stays for a downgraded build, and the fold is idempotent. + expect(entries.get("workspace:ws-child:dir/only.md")?.pinned).toBe(true); + await service.mergeKeys("workspace:ws-child:dir", "workspace:ws-owner:dir", { + pinned: "target", + }); + expect(await service.getEntries()).toEqual(entries); + // `pinned: "source"`: the child's pin overrides the owner's. + await service.mergeKeys("workspace:ws-child:dir/both.md", "workspace:ws-owner:dir/both.md", { + pinned: "source", + }); + entries = await service.getEntries(); + expect(entries.get("workspace:ws-owner:dir/both.md")?.pinned).toBe(true); + }); }); diff --git a/src/node/services/memoryMeta.ts b/src/node/services/memoryMeta.ts index 137e650e6c..5e2c086301 100644 --- a/src/node/services/memoryMeta.ts +++ b/src/node/services/memoryMeta.ts @@ -65,6 +65,12 @@ export interface MemoryMetaEntry { lastWriteAt: number | null; } +function maxTimestamp(a: number | null, b: number | null): number | null { + if (a === null) return b; + if (b === null) return a; + return Math.max(a, b); +} + const EMPTY_ENTRY: MemoryMetaEntry = { pinned: false, accessCount: 0, @@ -224,6 +230,38 @@ export class MemoryMetaService { } }), + /** + * Fold a subtree's entries into a second key, keeping the source: a + * legacy sub-agent note copied into the shared store stays readable by a + * downgraded build under its child key, so its pin/stats must too. A + * missing target entry is copied; an existing one keeps the larger + * counters/timestamps, and its pin either stands (`pinned: "target"`, a + * first adoption must not override the owner's own choice) or follows the + * source (`pinned: "source"`, the child changed it since the last + * adoption — see MemoryService.adoptLegacyPrivateStore). Idempotent. + */ + mergeKeys: ( + sourceLogicalKey: string, + targetLogicalKey: string, + options: { pinned: "target" | "source" } + ): Effect.Effect => + this.mutate((entries) => { + for (const [key, source] of Object.entries(entries)) { + if (!keyInSubtree(key, sourceLogicalKey)) continue; + const targetKey = `${targetLogicalKey}${key.slice(sourceLogicalKey.length)}`; + const target = entries[targetKey]; + entries[targetKey] = + target === undefined + ? { ...source } + : { + pinned: options.pinned === "source" ? source.pinned : target.pinned, + accessCount: Math.max(target.accessCount, source.accessCount), + lastAccessedAt: maxTimestamp(target.lastAccessedAt, source.lastAccessedAt), + lastWriteAt: maxTimestamp(target.lastWriteAt, source.lastWriteAt), + }; + } + }), + /** * Drop all entries for a deleted file or directory subtree so a future file * at the same path never resurrects stale pins or stats. @@ -247,25 +285,50 @@ export class MemoryMetaService { * (logged for diagnosis) and only writes can fail. */ private load(): Effect.Effect { + return Effect.map(this.loadWithHealth(), (loaded) => loaded.meta); + } + + /** + * `load()` plus whether this view is a healed substitute for a sidecar that + * exists but could not be read. Reads may serve that substitute; a mutation + * must not (see mutate()). + */ + private loadWithHealth(): Effect.Effect<{ meta: MemoryMetaFile; readFailed: boolean }> { // eslint-disable-next-line @typescript-eslint/no-this-alias -- Effect.gen generator bodies do not inherit `this` const self = this; return Effect.gen(function* () { - if (self.cache !== null) return self.cache; - const parsed = yield* Effect.tryPromise({ - try: async (): Promise => - JSON.parse(await fsPromises.readFile(self.metaPath, "utf-8")), + if (self.cache !== null) return { meta: self.cache, readFailed: false }; + let readFailed = false; + const raw = yield* Effect.tryPromise({ + try: (): Promise => fsPromises.readFile(self.metaPath, "utf-8"), catch: (error) => error, }).pipe( Effect.catch((error) => { // Missing file is the normal first-run case; anything else is healed to empty. if ((error as NodeJS.ErrnoException).code !== "ENOENT") { log.debug("[MemoryMetaService] healing unreadable sidecar", { error }); + readFailed = true; } - return Effect.succeed(null); + return Effect.succeed(null); }) ); - self.cache = sanitizeMetaFile(parsed); - return self.cache; + let parsed: unknown = null; + if (raw !== null) { + try { + parsed = JSON.parse(raw); + } catch (error) { + // Corrupt content (unlike a failed read) IS the file's state: healing + // it to empty and letting the next mutation rewrite it is the fix. + log.debug("[MemoryMetaService] healing corrupt sidecar", { error }); + } + } + const meta = sanitizeMetaFile(parsed); + // A transiently unreadable sidecar (EACCES interval, a writer mid-swap) + // heals to empty for THIS call only: caching that empty view would keep + // serving it once readable again — and the next mutation would write + // the pins and stats away. + if (!readFailed) self.cache = meta; + return { meta, readFailed }; }); } @@ -281,7 +344,19 @@ export class MemoryMetaService { const self = this; return this.writeLock.withPermit( Effect.gen(function* () { - const meta = yield* self.load(); + const { meta, readFailed } = yield* self.loadWithHealth(); + // A read that healed to empty is fine to serve, but rewriting the + // sidecar from it would erase every existing pin and usage stat the + // moment the file becomes readable again. Fail the mutation instead; + // the caller retries on a later call, which re-reads. + if (readFailed) { + return yield* Effect.fail( + new MemoryMetaWriteError({ + metaPath: self.metaPath, + reason: "sidecar exists but could not be read; refusing to overwrite it", + }) + ); + } const entries = { ...meta.entries }; update(entries); for (const [key, entry] of Object.entries(entries)) { @@ -327,6 +402,21 @@ export class MemoryMetaService { return Effect.runPromise(this.effects.getEntries()); } + /** + * `getEntries()` that refuses a healed substitute: throws when the sidecar + * exists but could not be read. For decisions that consume the entries + * destructively — the legacy-notebook handover before a sub-agent's session + * is deleted folds child-keyed pins/usage into the owner key; an empty + * substitute would fold nothing, report success, and strand the entries. + */ + async getEntriesOrThrow(): Promise> { + const { meta, readFailed } = await Effect.runPromise(this.loadWithHealth()); + if (readFailed) { + throw new Error(`memory metadata sidecar could not be read at ${this.metaPath}`); + } + return new Map(Object.entries(meta.entries).map(([key, entry]) => [key, { ...entry }])); + } + async setPinned(logicalKey: string, pinned: boolean): Promise { await Effect.runPromise(this.effects.setPinned(logicalKey, pinned)); } @@ -344,6 +434,15 @@ export class MemoryMetaService { await Effect.runPromise(this.effects.renameKeys(oldLogicalKey, newLogicalKey)); } + /** Fold a subtree's entries into `targetLogicalKey`, keeping the source (see effects). */ + async mergeKeys( + sourceLogicalKey: string, + targetLogicalKey: string, + options: { pinned: "target" | "source" } + ): Promise { + await Effect.runPromise(this.effects.mergeKeys(sourceLogicalKey, targetLogicalKey, options)); + } + /** * Drop all entries for a deleted file or directory subtree so a future file * at the same path never resurrects stale pins or stats. diff --git a/src/node/services/memoryService.test.ts b/src/node/services/memoryService.test.ts index 5c9b3b85f1..2d56da8d05 100644 --- a/src/node/services/memoryService.test.ts +++ b/src/node/services/memoryService.test.ts @@ -2,6 +2,7 @@ import { describe, it, expect, spyOn } from "bun:test"; import { MEMORY_MAX_FILES_PER_SCOPE, MEMORY_MAX_FILE_BYTES } from "@/common/constants/memory"; +import { execFileSync } from "node:child_process"; import { createHash } from "node:crypto"; import * as fsPromises from "node:fs/promises"; import * as path from "node:path"; @@ -26,6 +27,7 @@ import { } from "@/common/types/refinement"; import { applyRefinementInverse, readRefinementEvents } from "./refinement/refinementTestHelpers"; import { rollbackRefinement } from "./refinement/refinementRollback"; +import { legacyAdoptionManifestPath } from "./memoryLegacyAdoption"; import { workspaceRemovalTombstonePath } from "./workspaceRemoval"; import { TestTempDir } from "./tools/testHelpers"; @@ -1488,6 +1490,799 @@ describe("MemoryService", () => { expect((await fixture.metaService.getPinnedKeys()).size).toBe(0); expect(events).toEqual([]); }); + it("refuses a read whose workspace was tombstoned while the legacy adoption pass ran", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + await fixture.service.create(fixture.ctx, "/memories/workspace/n.md", "shared", "agent"); + // The adoption pass (owner-store lock) is the window: another backend's + // removal of ws-child publishes its tombstone after the readability + // check that opened the store, and the pass swallows its own refusal. + const tombstonePath = workspaceRemovalTombstonePath(fixture.xumHome, "ws-child"); + spyOn( + fixture.service as unknown as { adoptLegacyPrivateStore: () => Promise }, + "adoptLegacyPrivateStore" + ).mockImplementationOnce(async () => { + await fsPromises.mkdir(path.dirname(tombstonePath), { recursive: true }); + await fsPromises.writeFile(tombstonePath, JSON.stringify({ workspaceId: "ws-child" })); + }); + const refused = await fixture.service.view(fixture.ctx, "/memories/workspace/n.md"); + expect(refused.success).toBe(false); + if (!refused.success) expect(refused.error).toContain("was removed"); + }); + + it("adopts a sub-agent's pre-sharing private notebook into the shared store, keeping the legacy copy downgrade-readable", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + // Notes written by a build that kept the child's workspace scope in its + // own session dir, plus a pin recorded under the child's logical key. + const legacyRoot = path.join(fixture.config.sessionsDir, "ws-child", "memory"); + await fsPromises.mkdir(path.join(legacyRoot, "sub"), { recursive: true }); + await fsPromises.writeFile(path.join(legacyRoot, "only-child.md"), "child notes"); + await fsPromises.writeFile(path.join(legacyRoot, "sub", "same.md"), "identical"); + await fsPromises.writeFile(path.join(legacyRoot, "clash.md"), "child version"); + await fixture.metaService.setPinned("workspace:ws-child:only-child.md", true); + // The owner already holds one identical and one conflicting file + // (written before the upgrade: an owner access would adopt the child's + // notes first, see "owner access adopts ..." below). + const ownerCtx = { ...fixture.ctx, workspaceId: "ws-owner" }; + const ownerRoot = path.join(fixture.config.sessionsDir, "ws-owner", "memory"); + await fsPromises.mkdir(path.join(ownerRoot, "sub"), { recursive: true }); + await fsPromises.writeFile(path.join(ownerRoot, "sub", "same.md"), "identical"); + await fsPromises.writeFile(path.join(ownerRoot, "clash.md"), "owner version"); + const events: unknown[] = []; + fixture.service.on("change", (event) => events.push(event)); + + const listed = await fixture.service.listIndexEntries(fixture.ctx); + expect(listed.filter((e) => e.scope === "workspace").map((e) => e.relPath)).toEqual([ + "clash.md", + "imported/ws-child/clash.md", + "only-child.md", + "sub/same.md", + ]); + expect(await fsPromises.readFile(path.join(ownerRoot, "only-child.md"), "utf-8")).toBe( + "child notes" + ); + expect(await fsPromises.readFile(path.join(ownerRoot, "clash.md"), "utf-8")).toBe( + "owner version" + ); + expect( + await fsPromises.readFile(path.join(ownerRoot, "imported", "ws-child", "clash.md"), "utf-8") + ).toBe("child version"); + // The pin is copied to the owner-keyed logical key; the child-keyed + // entry stays for a downgraded build, which keys by the child id. + expect([...(await fixture.metaService.getPinnedKeys())].sort()).toEqual([ + "workspace:ws-child:only-child.md", + "workspace:ws-owner:only-child.md", + ]); + // The legacy copy stays where a downgraded build reads it; the tree's + // tabs were told once and a second access is a no-op. + expect(await fsPromises.readFile(path.join(legacyRoot, "only-child.md"), "utf-8")).toBe( + "child notes" + ); + expect(events).toEqual([ + { + scope: "workspace", + path: "/memories/workspace", + actor: "agent", + workspaceId: "ws-owner", + projectPath: FIXTURE_PROJECT_PATH, + }, + ]); + await fixture.service.listIndexEntries(fixture.ctx); + expect(events).toHaveLength(1); + // Visible to the owner through its own context, like any shared note. + const ownerView = await fixture.service.view(ownerCtx, "/memories/workspace/only-child.md"); + expect(ownerView.success).toBe(true); + if (ownerView.success) expect(ownerView.output).toContain("child notes"); + + // Edited through the shared store, then a backend restart: the legacy + // copy is known to be folded in already and must not resurface as a + // stale duplicate. + await fixture.service.strReplace( + ownerCtx, + "/memories/workspace/only-child.md", + "child notes", + "shared edit", + "agent" + ); + // A downgraded build wrote a new note into the legacy dir meanwhile. + await fsPromises.writeFile(path.join(legacyRoot, "downgrade.md"), "written on old build"); + // An earlier adoption was interrupted right after writing this file's + // bytes: identical bytes in the owner store, pin only under the child + // key, nothing in the manifest. The retry must still copy the pin. + await fsPromises.writeFile(path.join(legacyRoot, "half.md"), "half adopted"); + await fsPromises.writeFile(path.join(ownerRoot, "half.md"), "half adopted"); + await fixture.metaService.setPinned("workspace:ws-child:half.md", true); + // Other processes' sidecar writes are read through fresh instances: the + // fixture's own sidecar cache does not observe them (no cross-process + // stamp validation in this layer). + const meta = () => new MemoryMetaService(fixture.xumHome); + const restarted = new MemoryService(fixture.config, new MemoryMetaService(fixture.xumHome)); + const restartedEvents: unknown[] = []; + restarted.on("change", (event) => restartedEvents.push(event)); + const relisted = await restarted.listIndexEntries(fixture.ctx); + // The pass wrote one file and copied one pin: both change what the + // tree's readers derive from the store, so the tabs heard. + expect(restartedEvents).toHaveLength(1); + // Metadata-only pass (nothing to write, one pin to copy): same signals. + await fsPromises.writeFile(path.join(legacyRoot, "meta-only.md"), "same bytes"); + await fsPromises.writeFile(path.join(ownerRoot, "meta-only.md"), "same bytes"); + await meta().setPinned("workspace:ws-child:meta-only.md", true); + const metaOnly = new MemoryService(fixture.config, new MemoryMetaService(fixture.xumHome)); + const metaOnlyEvents: unknown[] = []; + metaOnly.on("change", (event) => metaOnlyEvents.push(event)); + await metaOnly.listIndexEntries(fixture.ctx); + expect(metaOnlyEvents).toHaveLength(1); + expect(await meta().getPinnedKeys()).toContain("workspace:ws-owner:meta-only.md"); + + // Sidecar-only changes made on a downgraded build (bytes untouched) are + // folded in on the next upgrade: an unpin of half.md under the child key + // reaches the owner key (its recorded target still holds the bytes)... + const freshService = () => + new MemoryService(fixture.config, new MemoryMetaService(fixture.xumHome)); + await meta().setPinned("workspace:ws-child:half.md", false); + await freshService().listIndexEntries(fixture.ctx); + expect(await meta().getPinnedKeys()).not.toContain("workspace:ws-owner:half.md"); + // ...while the owner's OWN later choice is not undone by an unchanged + // child entry on every restart. + await meta().setPinned("workspace:ws-owner:half.md", true); + await freshService().listIndexEntries(fixture.ctx); + expect(await meta().getPinnedKeys()).toContain("workspace:ws-owner:half.md"); + // only-child.md's recorded target was replaced by the shared edit: the + // child's pin change must not land on the owner's new content. The + // legacy note is placed anew (imported/) and carries the child's state. + await meta().setPinned("workspace:ws-child:only-child.md", false); + await freshService().listIndexEntries(fixture.ctx); + expect(await meta().getPinnedKeys()).toContain("workspace:ws-owner:only-child.md"); + expect( + await fsPromises.readFile( + path.join(ownerRoot, "imported", "ws-child", "only-child.md"), + "utf-8" + ) + ).toBe("child notes"); + expect(await meta().getPinnedKeys()).not.toContain( + "workspace:ws-owner:imported/ws-child/only-child.md" + ); + expect(relisted.filter((e) => e.scope === "workspace").map((e) => e.relPath)).toEqual([ + "clash.md", + "downgrade.md", + "half.md", + "imported/ws-child/clash.md", + "only-child.md", + "sub/same.md", + ]); + expect(await fsPromises.readFile(path.join(ownerRoot, "only-child.md"), "utf-8")).toBe( + "shared edit" + ); + expect([...(await meta().getPinnedKeys())].sort()).toEqual([ + "workspace:ws-child:meta-only.md", + "workspace:ws-owner:half.md", + "workspace:ws-owner:meta-only.md", + "workspace:ws-owner:only-child.md", + ]); + + // A workspace that is its own owner keeps its private store untouched. + const solo = { ...fixture.ctx, workspaceId: "ws-solo" }; + await fixture.service.create(solo, "/memories/workspace/mine.md", "solo", "agent"); + expect( + await pathExists(path.join(fixture.config.sessionsDir, "ws-solo", "memory", "mine.md")) + ).toBe(true); + }); + + it("stops adopting legacy notes at the shared store's remaining file capacity", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + const ownerRoot = path.join(fixture.config.sessionsDir, "ws-owner", "memory"); + const legacyRoot = path.join(fixture.config.sessionsDir, "ws-child", "memory"); + await fsPromises.mkdir(ownerRoot, { recursive: true }); + await fsPromises.mkdir(legacyRoot, { recursive: true }); + // Owner two below the cap; child brings five (one identical to an owner + // file, which needs no slot). + await Promise.all( + Array.from({ length: MEMORY_MAX_FILES_PER_SCOPE - 2 }, (_, i) => + fsPromises.writeFile(path.join(ownerRoot, `o${String(i).padStart(4, "0")}.md`), "o") + ) + ); + await fsPromises.writeFile(path.join(ownerRoot, "shared.md"), "same"); + for (const name of ["a.md", "b.md", "c.md", "d.md"]) { + await fsPromises.writeFile(path.join(legacyRoot, name), `child ${name}`); + } + await fsPromises.writeFile(path.join(legacyRoot, "shared.md"), "same"); + + const listed = await fixture.service.listIndexEntries(fixture.ctx); + const workspaceFiles = listed.filter((e) => e.scope === "workspace").map((e) => e.relPath); + // Exactly at the cap, never above: one slot was already taken by + // shared.md's owner copy, so only one of the four new notes fit. + expect(workspaceFiles).toHaveLength(MEMORY_MAX_FILES_PER_SCOPE); + expect(workspaceFiles.filter((f) => ["a.md", "b.md", "c.md", "d.md"].includes(f))).toEqual([ + "a.md", + ]); + // A create into the full scope is refused like before, so the invariant holds. + const full = await fixture.service.create( + fixture.ctx, + "/memories/workspace/new.md", + "x", + "agent" + ); + expect(full.success).toBe(false); + // Bounded: the incomplete pass is memoized against the legacy store's + // stamp like a complete one, so an over-cap notebook is not re-walked + // (manifest, sidecar, owner listing) on every access — freed capacity + // alone does not re-run it. + const passes = spyOn( + fixture.service as unknown as { readOrQuarantineAdoptionManifest: () => Promise }, + "readOrQuarantineAdoptionManifest" + ); + await fixture.service.deletePath({ ...fixture.ctx }, "/memories/workspace/o0000.md", "agent"); + await fixture.service.deletePath({ ...fixture.ctx }, "/memories/workspace/o0001.md", "agent"); + const relisted = (await fixture.service.listIndexEntries({ ...fixture.ctx })) + .filter((e) => e.scope === "workspace") + .map((e) => e.relPath); + expect(passes).not.toHaveBeenCalled(); + expect(relisted).toHaveLength(MEMORY_MAX_FILES_PER_SCOPE - 2); + expect(relisted.filter((f) => ["a.md", "b.md", "c.md", "d.md"].includes(f))).toEqual([ + "a.md", + ]); + // A changed legacy store (a note written on the downgraded build) + // re-runs the pass, which then uses the freed capacity. + await fsPromises.writeFile(path.join(legacyRoot, "e.md"), "child e.md"); + const rewalked = (await fixture.service.listIndexEntries({ ...fixture.ctx })) + .filter((e) => e.scope === "workspace") + .map((e) => e.relPath); + expect(passes).toHaveBeenCalledTimes(1); + expect(rewalked).toHaveLength(MEMORY_MAX_FILES_PER_SCOPE); + expect(rewalked.filter((f) => ["a.md", "b.md", "c.md", "d.md", "e.md"].includes(f))).toEqual([ + "a.md", + "b.md", + "c.md", + ]); + }); + + it("adopts addressable dot-entry notes and refuses the handover over unrepresentable ones", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + const ownerRoot = path.join(fixture.config.sessionsDir, "ws-owner", "memory"); + const legacyRoot = path.join(fixture.config.sessionsDir, "ws-child", "memory"); + // The path grammar admits dotfiles, so a downgraded child may hold a + // real note at `.note` that no listing ever showed — including one + // whose text `create` accepted but the lossy-decode gate cannot vouch + // for. Such an entry must hold up removal like a listed note would + // (r73), not be written off as a stray `.DS_Store`. + await fsPromises.mkdir(path.join(legacyRoot, ".hidden"), { recursive: true }); + await fsPromises.writeFile(path.join(legacyRoot, ".note"), "dot note"); + await fsPromises.writeFile(path.join(legacyRoot, ".hidden", "n.md"), "nested dot note"); + // Every in-namespace path is a note's, including one named like the + // pass's own staging area (which lives OUTSIDE the memory root, r91): + // adoption must never mistake it for staged bytes and remove it. + await fsPromises.mkdir(path.join(legacyRoot, "memory-adoption-staging"), { + recursive: true, + }); + await fsPromises.writeFile( + path.join(legacyRoot, "memory-adoption-staging", "n.md"), + "staging-named note" + ); + await fsPromises.writeFile( + path.join(legacyRoot, ".DS_Store"), + Buffer.from([0, 0, 1, 255, 254]) + ); + expect( + await fixture.service + .adoptLegacyPrivateStoreForRemoval("ws-child", "ws-owner") + .then(() => null, getErrorMessage) + ).toMatch(/1 legacy workspace memory note\(s\)/); + // The representable dot-entries were folded in by that same pass. + expect(await fsPromises.readFile(path.join(ownerRoot, ".note"), "utf-8")).toBe("dot note"); + expect(await fsPromises.readFile(path.join(ownerRoot, ".hidden", "n.md"), "utf-8")).toBe( + "nested dot note" + ); + expect(await pathExists(path.join(ownerRoot, ".DS_Store"))).toBe(false); + expect( + await fsPromises.readFile(path.join(ownerRoot, "memory-adoption-staging", "n.md"), "utf-8") + ).toBe("staging-named note"); + // Removing the stray entry lets a retried (non-forced) handover complete. + await fsPromises.rm(path.join(legacyRoot, ".DS_Store")); + await fixture.service.adoptLegacyPrivateStoreForRemoval("ws-child", "ws-owner"); + // Still addressable through the shared store, like on the old build. + const viewed = await fixture.service.view(fixture.ctx, "/memories/workspace/.note"); + expect(viewed.success).toBe(true); + if (viewed.success) expect(viewed.output).toContain("dot note"); + }); + + it("adopts the legacy notebook for removal without any prior access, and throws instead of deferring", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + const ownerRoot = path.join(fixture.config.sessionsDir, "ws-owner", "memory"); + const legacyRoot = path.join(fixture.config.sessionsDir, "ws-child", "memory"); + await fsPromises.mkdir(legacyRoot, { recursive: true }); + await fsPromises.writeFile(path.join(legacyRoot, "only-child.md"), "child notes"); + // No workspace-memory entry point ever served ws-child in this process: + // removal's handover must fold the notes in by itself. + await fixture.service.adoptLegacyPrivateStoreForRemoval("ws-child", "ws-owner"); + expect(await fsPromises.readFile(path.join(ownerRoot, "only-child.md"), "utf-8")).toBe( + "child notes" + ); + // Idempotent: a retried removal re-runs the pass (the per-process memo + // is bypassed) and finds nothing new. + await fixture.service.adoptLegacyPrivateStoreForRemoval("ws-child", "ws-owner"); + // A removal that verified a different owner than the store now resolves + // to must not adopt into the wrong notebook. + expect( + await fixture.service + .adoptLegacyPrivateStoreForRemoval("ws-child", "ws-other") + .then(() => null, getErrorMessage) + ).toMatch(/resolved to ws-owner/); + // Failures surface (the access-time pass only logs and retries later). + // A sidecar that exists but cannot be read is no "no metadata": the + // handover would copy the note without its pin and report success. + await fsPromises.writeFile(path.join(legacyRoot, "late.md"), "written later"); + const metaPath = path.join(fixture.xumHome, "memory-meta.json"); + const savedMeta = await fsPromises.readFile(metaPath).catch(() => null); + await fsPromises.rm(metaPath, { force: true }); + await fsPromises.mkdir(metaPath); // EISDIR on read + // A service whose sidecar cache is cold (a restarted backend) reads the + // file: the strict read refuses instead of healing to "no metadata". + const coldSidecar = new MemoryService(fixture.config, new MemoryMetaService(fixture.xumHome)); + try { + expect( + await coldSidecar + .adoptLegacyPrivateStoreForRemoval("ws-child", "ws-owner") + .then(() => null, getErrorMessage) + ).toMatch(/sidecar could not be read/); + } finally { + await fsPromises.rmdir(metaPath); + if (savedMeta !== null) await fsPromises.writeFile(metaPath, savedMeta); + } + expect(await pathExists(path.join(ownerRoot, "late.md"))).toBe(false); + // Same for the adoption manifest: unreadable (not missing) aborts. + const manifestPath = legacyAdoptionManifestPath(path.dirname(legacyRoot)); + const savedManifest = await fsPromises.readFile(manifestPath); + await fsPromises.rm(manifestPath); + await fsPromises.mkdir(manifestPath); + try { + expect( + await fixture.service + .adoptLegacyPrivateStoreForRemoval("ws-child", "ws-owner") + .then(() => null, getErrorMessage) + ).toMatch(/EISDIR/); + } finally { + await fsPromises.rmdir(manifestPath); + await fsPromises.writeFile(manifestPath, savedManifest); + } + expect(await pathExists(path.join(ownerRoot, "late.md"))).toBe(false); + // A MALFORMED (not merely unreadable) manifest self-heals: it is + // quarantined beside itself and the pass re-adopts from scratch + // (idempotent: identical files are skipped), so neither access-time + // adoption nor a non-forced removal is blocked forever by it. + const quarantined = async () => + (await fsPromises.readdir(path.dirname(legacyRoot))).filter((name) => + name.startsWith(`${path.basename(manifestPath)}.malformed-`) + ); + for (const body of ["{nope", "[]", JSON.stringify({ "note.md": { content: 1 } })]) { + await fsPromises.writeFile(manifestPath, body); + await fixture.service.adoptLegacyPrivateStoreForRemoval("ws-child", "ws-owner"); + } + expect((await quarantined()).length).toBe(3); + expect(await pathExists(path.join(ownerRoot, "late.md"))).toBe(true); + expect(await pathExists(path.join(ownerRoot, "only-child.md"))).toBe(true); + // The rewritten manifest records the re-adopted notes. + expect( + Object.keys(JSON.parse(await fsPromises.readFile(manifestPath, "utf-8")) as object).sort() + ).toEqual(["late.md", "only-child.md"]); + // When the quarantine rename itself fails, the pass fails closed like before. + await fsPromises.writeFile(manifestPath, "{nope"); + const rename = spyOn(fsPromises, "rename").mockImplementationOnce(() => + Promise.reject(Object.assign(new Error("EACCES"), { code: "EACCES" })) + ); + try { + expect( + await fixture.service + .adoptLegacyPrivateStoreForRemoval("ws-child", "ws-owner") + .then(() => null, getErrorMessage) + ).toContain("malformed (not JSON)"); + } finally { + rename.mockRestore(); + } + expect(await fsPromises.readFile(manifestPath, "utf-8")).toBe("{nope"); + expect((await quarantined()).length).toBe(3); + await fsPromises.writeFile(manifestPath, savedManifest); + await fixture.service.adoptLegacyPrivateStoreForRemoval("ws-child", "ws-owner"); + expect(await pathExists(path.join(ownerRoot, "late.md"))).toBe(true); + }); + + it("retries a transiently unreadable legacy note on the next access, but not a permanently skipped one", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + const ownerRoot = path.join(fixture.config.sessionsDir, "ws-owner", "memory"); + const legacyRoot = path.join(fixture.config.sessionsDir, "ws-child", "memory"); + await fsPromises.mkdir(legacyRoot, { recursive: true }); + await fsPromises.writeFile(path.join(legacyRoot, "flaky.md"), "readable later"); + await fsPromises.writeFile(path.join(legacyRoot, "fine.md"), "fine"); + const passes = spyOn( + fixture.service as unknown as { readOrQuarantineAdoptionManifest: () => Promise }, + "readOrQuarantineAdoptionManifest" + ); + // A permission interval on one note (EACCES on open): the pass adopts + // the rest and must NOT memoize — the same legacy store can yield more + // once the failure clears. + const realOpen = fsPromises.open.bind(fsPromises); + const flakyOpen = spyOn(fsPromises, "open").mockImplementation(((target, ...rest) => + String(target).endsWith(path.join("memory", "flaky.md")) + ? Promise.reject( + Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + ) + : realOpen(target as string, ...(rest as []))) as typeof fsPromises.open); + try { + await fixture.service.listIndexEntries({ ...fixture.ctx }); + } finally { + flakyOpen.mockRestore(); + } + expect(passes).toHaveBeenCalledTimes(1); + expect(await pathExists(path.join(ownerRoot, "fine.md"))).toBe(true); + expect(await pathExists(path.join(ownerRoot, "flaky.md"))).toBe(false); + // Failure cleared, legacy store unchanged: the next access retries. + await fixture.service.listIndexEntries({ ...fixture.ctx }); + expect(passes).toHaveBeenCalledTimes(2); + expect(await fsPromises.readFile(path.join(ownerRoot, "flaky.md"), "utf-8")).toBe( + "readable later" + ); + // Complete now: memoized. + await fixture.service.listIndexEntries({ ...fixture.ctx }); + expect(passes).toHaveBeenCalledTimes(2); + + // A PERMANENT skip (a note the owner store cannot represent: both + // destinations hold different content) is memoized like a complete + // pass — an unchanged legacy store cannot adopt it on a retry. + await fsPromises.writeFile(path.join(legacyRoot, "clash.md"), "child"); + await fsPromises.writeFile(path.join(ownerRoot, "clash.md"), "owner"); + await fsPromises.mkdir(path.join(ownerRoot, "imported", "ws-child"), { recursive: true }); + await fsPromises.writeFile(path.join(ownerRoot, "imported", "ws-child", "clash.md"), "other"); + await fixture.service.listIndexEntries({ ...fixture.ctx }); + expect(passes).toHaveBeenCalledTimes(3); + await fixture.service.listIndexEntries({ ...fixture.ctx }); + expect(passes).toHaveBeenCalledTimes(3); + }); + + it("adopts a legacy note containing a literal U+FFFD and skips invalid UTF-8 as unrepresentable", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + const ownerRoot = path.join(fixture.config.sessionsDir, "ws-owner", "memory"); + const legacyRoot = path.join(fixture.config.sessionsDir, "ws-child", "memory"); + await fsPromises.mkdir(legacyRoot, { recursive: true }); + // A replacement character the author actually typed is valid UTF-8. + await fsPromises.writeFile(path.join(legacyRoot, "marker.md"), "decoded as \uFFFD here"); + // Bytes that are not UTF-8 at all cannot be carried by a text write. + await fsPromises.writeFile( + path.join(legacyRoot, "binary.md"), + Buffer.from([0xff, 0xfe, 0x41]) + ); + expect( + await fixture.service + .adoptLegacyPrivateStoreForRemoval("ws-child", "ws-owner") + .then(() => null, getErrorMessage) + ).toMatch(/1 legacy workspace memory note\(s\)/); + expect(await fsPromises.readFile(path.join(ownerRoot, "marker.md"), "utf-8")).toBe( + "decoded as \uFFFD here" + ); + expect(await pathExists(path.join(ownerRoot, "binary.md"))).toBe(false); + // Without the binary stray, the handover completes. + await fsPromises.rm(path.join(legacyRoot, "binary.md")); + await fixture.service.adoptLegacyPrivateStoreForRemoval("ws-child", "ws-owner"); + }); + + it("never opens a non-regular entry at a destination: a FIFO there is occupied, not read", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + const ownerRoot = path.join(fixture.config.sessionsDir, "ws-owner", "memory"); + const legacyRoot = path.join(fixture.config.sessionsDir, "ws-child", "memory"); + await fsPromises.mkdir(legacyRoot, { recursive: true }); + await fsPromises.mkdir(path.join(ownerRoot, "imported", "ws-child"), { recursive: true }); + await fsPromises.writeFile(path.join(legacyRoot, "note.md"), "child"); + await fsPromises.writeFile(path.join(legacyRoot, "stuck.md"), "child too"); + await fsPromises.writeFile(path.join(ownerRoot, "imported", "ws-child", "stuck.md"), "other"); + try { + execFileSync("mkfifo", [path.join(ownerRoot, "note.md"), path.join(ownerRoot, "stuck.md")]); + } catch { + return; // no mkfifo here (non-POSIX host): nothing to exercise + } + const passes = spyOn( + fixture.service as unknown as { readOrQuarantineAdoptionManifest: () => Promise }, + "readOrQuarantineAdoptionManifest" + ); + // Opening a FIFO for reading blocks until a writer shows up; the pass + // must settle without one and treat the entry as owner state. + await fixture.service.listIndexEntries({ ...fixture.ctx }); + expect((await fsPromises.lstat(path.join(ownerRoot, "note.md"))).isFIFO()).toBe(true); + expect( + await fsPromises.readFile(path.join(ownerRoot, "imported", "ws-child", "note.md"), "utf-8") + ).toBe("child"); + // Both slots occupied: a PERMANENT skip (memoized, refused by removal), + // not a hang and not a retry loop. + expect( + (await fsPromises.readdir(path.join(ownerRoot, "imported", "ws-child"))).sort() + ).toEqual(["note.md", "stuck.md"]); + await fixture.service.listIndexEntries({ ...fixture.ctx }); + expect(passes).toHaveBeenCalledTimes(1); + expect( + await fixture.service + .adoptLegacyPrivateStoreForRemoval("ws-child", "ws-owner") + .then(() => null, getErrorMessage) + ).toMatch(/1 legacy workspace memory note\(s\)/); + }); + + it("compares destination bytes strictly: a legacy U+FFFD note is not settled by an invalid-UTF-8 owner note", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + const ownerRoot = path.join(fixture.config.sessionsDir, "ws-owner", "memory"); + const legacyRoot = path.join(fixture.config.sessionsDir, "ws-child", "memory"); + await fsPromises.mkdir(legacyRoot, { recursive: true }); + await fsPromises.mkdir(ownerRoot, { recursive: true }); + await fsPromises.writeFile(path.join(legacyRoot, "marker.md"), "decoded as \uFFFD here"); + // Lossily decoded, this reads as exactly the legacy text. + const ownerBytes = Buffer.concat([ + Buffer.from("decoded as "), + Buffer.from([0xff]), + Buffer.from(" here"), + ]); + await fsPromises.writeFile(path.join(ownerRoot, "marker.md"), ownerBytes); + // Complete handover: the note is represented byte-exact under the + // import directory, the owner's entry untouched. + await fixture.service.adoptLegacyPrivateStoreForRemoval("ws-child", "ws-owner"); + expect( + await fsPromises.readFile( + path.join(ownerRoot, "imported", "ws-child", "marker.md"), + "utf-8" + ) + ).toBe("decoded as \uFFFD here"); + expect( + (await fsPromises.readFile(path.join(ownerRoot, "marker.md"))).equals(ownerBytes) + ).toBe(true); + }); + + it("treats an unreadable destination as transient: no copy, no record, retried on the next access", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + const ownerRoot = path.join(fixture.config.sessionsDir, "ws-owner", "memory"); + const legacyRoot = path.join(fixture.config.sessionsDir, "ws-child", "memory"); + await fsPromises.mkdir(legacyRoot, { recursive: true }); + await fsPromises.mkdir(ownerRoot, { recursive: true }); + await fsPromises.writeFile(path.join(legacyRoot, "note.md"), "child"); + await fsPromises.writeFile(path.join(ownerRoot, "note.md"), "owner"); + const passes = spyOn( + fixture.service as unknown as { readOrQuarantineAdoptionManifest: () => Promise }, + "readOrQuarantineAdoptionManifest" + ); + // EACCES on the owner's note: neither free nor different — a + // mismatch verdict would duplicate the child's note under imported/. + const realOpen = fsPromises.open.bind(fsPromises); + const denied = spyOn(fsPromises, "open").mockImplementation(((target, ...rest) => + String(target).endsWith(path.join("ws-owner", "memory", "note.md")) + ? Promise.reject( + Object.assign(new Error("EACCES: permission denied"), { code: "EACCES" }) + ) + : realOpen(target as string, ...(rest as []))) as typeof fsPromises.open); + try { + await fixture.service.listIndexEntries({ ...fixture.ctx }); + } finally { + denied.mockRestore(); + } + expect(passes).toHaveBeenCalledTimes(1); + expect(await pathExists(path.join(ownerRoot, "imported"))).toBe(false); + expect(await pathExists(legacyAdoptionManifestPath(path.dirname(legacyRoot)))).toBe(false); + // Cleared: the next access re-runs the pass and settles the note. + await fixture.service.listIndexEntries({ ...fixture.ctx }); + expect(passes).toHaveBeenCalledTimes(2); + expect( + await fsPromises.readFile(path.join(ownerRoot, "imported", "ws-child", "note.md"), "utf-8") + ).toBe("child"); + expect(await fsPromises.readFile(path.join(ownerRoot, "note.md"), "utf-8")).toBe("owner"); + await fixture.service.listIndexEntries({ ...fixture.ctx }); + expect(passes).toHaveBeenCalledTimes(2); + }); + + it("never copies a legacy note whose name the memory path grammar rejects", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + const ownerRoot = path.join(fixture.config.sessionsDir, "ws-owner", "memory"); + const legacyRoot = path.join(fixture.config.sessionsDir, "ws-child", "memory"); + await fsPromises.mkdir(legacyRoot, { recursive: true }); + await fsPromises.writeFile(path.join(legacyRoot, "bad..name.md"), "traversal-looking"); + await fsPromises.writeFile(path.join(legacyRoot, "ctl\u0001.md"), "control char"); + await fsPromises.writeFile(path.join(legacyRoot, "good.md"), "fine"); + const passes = spyOn( + fixture.service as unknown as { readOrQuarantineAdoptionManifest: () => Promise }, + "readOrQuarantineAdoptionManifest" + ); + await fixture.service.listIndexEntries({ ...fixture.ctx }); + expect(await fsPromises.readFile(path.join(ownerRoot, "good.md"), "utf-8")).toBe("fine"); + expect(await fsPromises.readdir(ownerRoot)).toEqual(["good.md"]); + // Permanent: an unchanged legacy store is not re-walked for them. + await fixture.service.listIndexEntries({ ...fixture.ctx }); + expect(passes).toHaveBeenCalledTimes(1); + // Strict removal still refuses to leave them behind. + expect( + await fixture.service + .adoptLegacyPrivateStoreForRemoval("ws-child", "ws-owner") + .then(() => null, getErrorMessage) + ).toMatch(/2 legacy workspace memory note\(s\)/); + expect(await fsPromises.readdir(ownerRoot)).toEqual(["good.md"]); + }); + + it("owner access adopts an inactive pre-sharing child's notebook without the child touching memory", async () => { + using fixture = await createFixture("ws-owner"); + await registerTaskTree(fixture); + // A sub-agent that finished before the upgrade: its private notebook + // exists, but no memory command will ever run in its context again. + const legacyRoot = path.join(fixture.config.sessionsDir, "ws-grandchild", "memory"); + await fsPromises.mkdir(legacyRoot, { recursive: true }); + await fsPromises.writeFile(path.join(legacyRoot, "finished.md"), "found before the upgrade"); + await fixture.metaService.setPinned("workspace:ws-grandchild:finished.md", true); + const events: unknown[] = []; + fixture.service.on("change", (event) => events.push(event)); + const passes = spyOn( + fixture.service as unknown as { readOrQuarantineAdoptionManifest: () => Promise }, + "readOrQuarantineAdoptionManifest" + ); + + // The OWNER's own access folds the descendant's notes in (one pass for + // the one child with a legacy root; ws-child and ws-solo have none). + const listed = await fixture.service.listIndexEntries(fixture.ctx); + expect(listed.filter((e) => e.scope === "workspace").map((e) => e.relPath)).toEqual([ + "finished.md", + ]); + expect(passes).toHaveBeenCalledTimes(1); + expect( + await fsPromises.readFile( + path.join(fixture.config.sessionsDir, "ws-owner", "memory", "finished.md"), + "utf-8" + ) + ).toBe("found before the upgrade"); + expect(await fixture.metaService.getPinnedKeys()).toContain("workspace:ws-owner:finished.md"); + expect(events).toHaveLength(1); + // The legacy copy stays for a downgraded build. + expect(await pathExists(path.join(legacyRoot, "finished.md"))).toBe(true); + + // A second owner access is a no-op: the memo answers for the unchanged + // legacy store (one lstat), no pass runs and nothing is announced. + await fixture.service.listIndexEntries({ ...fixture.ctx }); + expect(passes).toHaveBeenCalledTimes(1); + expect(events).toHaveLength(1); + // An unrelated root (ws-solo) never enumerates the tree's children. + await fixture.service.listIndexEntries({ ...fixture.ctx, workspaceId: "ws-solo" }); + expect(passes).toHaveBeenCalledTimes(1); + }); + + it("keeps adopting a legacy note named __proto__ exactly once", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + const ownerRoot = path.join(fixture.config.sessionsDir, "ws-owner", "memory"); + const legacyRoot = path.join(fixture.config.sessionsDir, "ws-child", "memory"); + await fsPromises.mkdir(legacyRoot, { recursive: true }); + await fsPromises.writeFile(path.join(legacyRoot, "__proto__"), "proto notes"); + await fixture.service.listIndexEntries({ ...fixture.ctx }); + expect(await fsPromises.readFile(path.join(ownerRoot, "__proto__"), "utf-8")).toBe( + "proto notes" + ); + const manifest = JSON.parse( + await fsPromises.readFile(legacyAdoptionManifestPath(path.dirname(legacyRoot)), "utf-8") + ) as Record; + expect(Object.keys(manifest)).toEqual(["__proto__"]); + // A fresh process (empty memo) finds the record and adopts nothing anew. + const restarted = new MemoryService(fixture.config, new MemoryMetaService(fixture.xumHome)); + const events: unknown[] = []; + restarted.on("change", (event) => events.push(event)); + await restarted.listIndexEntries({ ...fixture.ctx }); + expect(events).toEqual([]); + }); + + it("folds in a note written under a self-fallback once ownership resolves to the tree root again", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + // Shared access first: the (absent) legacy store is checked against ws-owner. + expect((await fixture.service.listIndexEntries(fixture.ctx)).length).toBe(0); + // config.json goes missing: the child resolves to itself and writes a + // note into its private dir. + const configPath = path.join(fixture.xumHome, "config.json"); + const savedConfig = await fsPromises.readFile(configPath); + await fsPromises.rm(configPath); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-child")).toBe("ws-child"); + const created = await fixture.service.create( + fixture.ctx, + "/memories/workspace/fallback.md", + "written while config was gone", + "agent" + ); + expect(created.success).toBe(true); + expect( + await pathExists(path.join(fixture.config.sessionsDir, "ws-child", "memory", "fallback.md")) + ).toBe(true); + // Config recovers: the same process must fold that note into the + // shared store instead of trusting its earlier "nothing to adopt". + await fsPromises.writeFile(configPath, savedConfig); + expect(fixture.service.resolveWorkspaceMemoryOwnerId("ws-child")).toBe("ws-owner"); + // Index builds get their own context object in production (the owner + // cache is per context); mirror that instead of reusing the command's. + const listed = await fixture.service.listIndexEntries({ ...fixture.ctx }); + expect(listed.filter((e) => e.scope === "workspace").map((e) => e.relPath)).toEqual([ + "fallback.md", + ]); + expect( + await fsPromises.readFile( + path.join(fixture.config.sessionsDir, "ws-owner", "memory", "fallback.md"), + "utf-8" + ) + ).toBe("written while config was gone"); + + // ANOTHER backend hits the same fallback while this process's resolution + // never changes: its write changes the legacy store's stamp, which this + // process notices on its next access and folds the note in. + const foreign = new MemoryService(fixture.config, new MemoryMetaService(fixture.xumHome)); + spyOn(foreign, "resolveWorkspaceMemoryOwnerId").mockReturnValue("ws-child"); + const foreignWrite = await foreign.create( + { ...fixture.ctx }, + "/memories/workspace/foreign.md", + "written by another backend's fallback", + "agent" + ); + expect(foreignWrite.success).toBe(true); + const afterForeign = await fixture.service.listIndexEntries({ ...fixture.ctx }); + expect(afterForeign.filter((e) => e.scope === "workspace").map((e) => e.relPath)).toEqual([ + "fallback.md", + "foreign.md", + ]); + }); + + it("never imports through a symlinked legacy notebook root or escaped files", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + const outside = path.join(fixture.xumHome, "outside"); + await fsPromises.mkdir(outside, { recursive: true }); + await fsPromises.writeFile(path.join(outside, "secret.md"), "host file"); + const childSessionDir = path.join(fixture.config.sessionsDir, "ws-child"); + await fsPromises.mkdir(childSessionDir, { recursive: true }); + // Root itself is a symlink: refused outright (lstat, never followed). + await fsPromises.symlink(outside, path.join(childSessionDir, "memory")); + expect( + (await fixture.service.listIndexEntries(fixture.ctx)).filter((e) => e.scope === "workspace") + ).toEqual([]); + + // Destination side: the owner store's imported/ component is a + // symlink out of the root. A conflicting legacy note would land there; + // the write is refused (and nothing is written outside), the note stays + // in the legacy dir unrecorded. + await fsPromises.unlink(path.join(childSessionDir, "memory")); + const legacyRoot = path.join(childSessionDir, "memory"); + await fsPromises.mkdir(legacyRoot); + await fsPromises.writeFile(path.join(legacyRoot, "clash.md"), "child version"); + const ownerRoot = path.join(fixture.config.sessionsDir, "ws-owner", "memory"); + await fsPromises.mkdir(path.join(ownerRoot, "imported"), { recursive: true }); + await fsPromises.writeFile(path.join(ownerRoot, "clash.md"), "owner version"); + await fsPromises.symlink(outside, path.join(ownerRoot, "imported", "ws-child")); + const escaped = new MemoryService(fixture.config, new MemoryMetaService(fixture.xumHome)); + expect( + (await escaped.listIndexEntries(fixture.ctx)) + .filter((e) => e.scope === "workspace") + .map((e) => e.relPath) + ).toEqual(["clash.md"]); + expect(await pathExists(path.join(outside, "clash.md"))).toBe(false); + expect(await pathExists(legacyAdoptionManifestPath(path.dirname(legacyRoot)))).toBe(false); + await fsPromises.unlink(path.join(ownerRoot, "imported", "ws-child")); + await fsPromises.rm(legacyRoot, { recursive: true }); + await fsPromises.rm(path.join(ownerRoot, "clash.md")); + + // Real root whose entries point outside: symlinked entries are not + // regular files to the walk, and a symlinked subdirectory is never + // descended into. + await fsPromises.mkdir(legacyRoot); + await fsPromises.symlink(path.join(outside, "secret.md"), path.join(legacyRoot, "link.md")); + await fsPromises.symlink(outside, path.join(legacyRoot, "linked-dir")); + await fsPromises.writeFile(path.join(legacyRoot, "real.md"), "real note"); + const fresh = new MemoryService(fixture.config, new MemoryMetaService(fixture.xumHome)); + expect( + (await fresh.listIndexEntries(fixture.ctx)) + .filter((e) => e.scope === "workspace") + .map((e) => e.relPath) + ).toEqual(["real.md"]); + }); }); describe("memory index entries", () => { diff --git a/src/node/services/memoryService.ts b/src/node/services/memoryService.ts index bd6ee6d380..8ae65ce29d 100644 --- a/src/node/services/memoryService.ts +++ b/src/node/services/memoryService.ts @@ -17,13 +17,14 @@ * documented limitation. */ import { EventEmitter } from "events"; -import { createHash } from "node:crypto"; +import { createHash, randomUUID } from "node:crypto"; import * as fsPromises from "node:fs/promises"; import * as path from "node:path"; import writeFileAtomic from "write-file-atomic"; import YAML from "yaml"; import assert from "@/common/utils/assert"; import { CONTEXT_NOTES_MEMORY_PATH } from "@/common/constants/contextBudget"; +import { hasErrorCode } from "@/node/services/tools/skillFileUtils"; import { MEMORY_HOT_SET_MAX_ITEM_BYTES, MEMORY_INDEX_DESCRIPTION_MAX_CHARS, @@ -47,6 +48,13 @@ import { withTargetMutationLock, } from "@/node/services/refinement/targetMutationLocks"; import { memoryLogicalKey, type MemoryMetaService } from "@/node/services/memoryMeta"; +import { + adoptionTargetStamp, + legacyAdoptionManifestPath, + LegacyAdoptionManifestMalformedError, + readLegacyAdoptionManifest, + type LegacyAdoptionRecord, +} from "@/node/services/memoryLegacyAdoption"; import { resolveWorkspaceMemoryOwnerId, workspaceMemoryOwnerResolver, @@ -378,8 +386,23 @@ interface MemoryStore { /** assertRootSafe + create the root if missing (write paths only). */ ensureRoot(): Promise; /** Relative paths of all non-dotfile files under the root, sorted. */ - listFiles(): Promise; - kind(relPath: string): Promise; + /** + * Files under the root. Tolerant and bounded by default (self-healing: an + * unreadable directory lists as empty; the walk stops past the per-scope + * cap); `strict` throws on any traversal failure and is unbounded, for + * callers whose decision must not rest on a possibly partial listing. + * Dot-entries are omitted unless `includeDotfiles`: listings and the index + * hide them, but the path grammar admits them, so a note such as `.note` + * is addressable — the legacy adoption pass must see it or removal would + * delete the only copy. + */ + listFiles(options?: { strict?: boolean; includeDotfiles?: boolean }): Promise; + /** + * Kind of an entry, null when absent. Tolerant by default (any stat failure + * reads as absent); `strict` throws unless the absence is proven (ENOENT / + * ENOTDIR), for callers about to overwrite whatever is there. + */ + kind(relPath: string, options?: { strict?: boolean }): Promise; /** * Read at most `maxBytes` from the head of the file. Index/hot-set builds * use this so files edited outside MemoryService cannot force unbounded reads @@ -387,6 +410,8 @@ interface MemoryStore { * the result as a best-effort prefix. */ readFilePrefix(relPath: string, maxBytes: number): Promise; + /** The same bounded prefix as raw bytes, for callers that must validate the encoding themselves. */ + readFilePrefixBytes(relPath: string, maxBytes: number): Promise; /** Atomic write; creates parent directories. */ writeFile(relPath: string, content: string): Promise; /** Recursive delete of a file or directory. */ @@ -400,6 +425,92 @@ interface MemoryStore { assertContained(relPath: string): Promise; } +const LEGACY_IMPORT_DIR = "imported"; +/** + * Directory beside the owner's memory root (in its session dir, OUTSIDE the + * model-writable memory namespace — a legacy note may legitimately live under + * any in-namespace path, dot-entries included) where the adoption pass stages + * a copy's bytes before installing them by rename + * (adoptLegacyPrivateStoreOrThrow); emptied at the start of every pass. + */ +const LEGACY_ADOPTION_STAGING_DIR_NAME = "memory-adoption-staging"; + +function legacyAdoptionStagingDir(store: MemoryStore): string { + return path.join(path.dirname(store.physicalRoot), LEGACY_ADOPTION_STAGING_DIR_NAME); +} + +/** + * Pin bit of a manifest record's child sidecar fingerprint. No child entry at + * that adoption is the default, unpinned state (a usage entry a downgraded + * build creates by merely viewing the note is not a pin transition); null + * only for an unparsable fingerprint. + */ +function legacySidecarPinned(sidecar: string): boolean | null { + if (sidecar === "") return false; + try { + const parsed: unknown = JSON.parse(sidecar); + return typeof parsed === "object" && parsed !== null + ? ((parsed as { pinned?: unknown }).pinned ?? false) === true + : null; + } catch { + return null; + } +} + +/** + * Change stamp of a sub-agent's legacy private store: the root directory's + * mtime and every listed file's size + mtime — a DOWNGRADED build editing an + * existing nested note moves the root mtime no more than a foreign backend's + * self-fallback write does. Bounded by the per-scope file cap and paid only + * while a legacy directory exists: an over-cap legacy store fingerprints + * only the capped prefix of its listing (an edit to a note sorted past it is + * picked up by a restart or removal's forced pass); the throttled + * full-store fingerprint lands with the multi-backend layer. Missing pieces + * read as fixed tokens. + */ +async function legacyStoreStamp(legacyRoot: string): Promise { + const rootMtime = await fsPromises + .stat(legacyRoot) + .then((stat) => String(stat.mtimeMs)) + .catch(() => "missing"); + const files = await new LocalMemoryStore(legacyRoot) + .listFiles({ includeDotfiles: true }) + .catch(() => []); + const fileStamps = await Promise.all( + files.map(async (relPath) => { + const stamp = await fsPromises + .lstat(path.join(legacyRoot, relPath), { bigint: true }) + .then((stat) => `${stat.size}:${stat.mtimeNs}`) + .catch(() => "missing"); + return `${relPath}=${stamp}`; + }) + ); + return `${rootMtime}:${fileStamps.join("\u0001")}`; +} + +/** A stat failure that proves the path is absent (vs. one that says nothing about it). */ +function isMissingPathError(error: unknown): boolean { + const code = (error as NodeJS.ErrnoException | null)?.code; + return code === "ENOENT" || code === "ENOTDIR"; +} + +/** + * Link-aware kind of a path: symlinks are reported as such, never followed. + * "missing" only when proven (ENOENT/ENOTDIR); any other failure (EACCES, + * EIO) is "unreadable" — a legacy notebook whose root cannot be inspected + * must not read as "nothing to adopt" to a removal about to delete it. + */ +async function lstatKind( + absPath: string +): Promise<"dir" | "symlink" | "other" | "missing" | "unreadable"> { + try { + const stat = await fsPromises.lstat(absPath); + return stat.isSymbolicLink() ? "symlink" : stat.isDirectory() ? "dir" : "other"; + } catch (error) { + return isMissingPathError(error) ? "missing" : "unreadable"; + } +} + function isPathWithinRoot( realRoot: string, candidate: string, @@ -442,16 +553,24 @@ class LocalMemoryStore implements MemoryStore { await fsPromises.mkdir(this.physicalRoot, { recursive: true }); } - async listFiles(): Promise { + async listFiles(options?: { strict?: boolean; includeDotfiles?: boolean }): Promise { const results: string[] = []; const walk = async (dirRel: string): Promise => { // Bounded walk: files may have been edited outside MemoryService. +1 lets - // callers detect overflow (e.g. the index logs its truncation). - if (results.length > MEMORY_MAX_FILES_PER_SCOPE) return; + // callers detect overflow (e.g. the index logs its truncation). Strict + // callers need the COMPLETE set (an omitted file would silently count + // as "nothing to adopt" and could lose its only copy), so the bound + // does not apply to them. + if (options?.strict !== true && results.length > MEMORY_MAX_FILES_PER_SCOPE) return; let entries; try { entries = await fsPromises.readdir(this.abs(dirRel), { withFileTypes: true }); - } catch { + } catch (error) { + // Strict callers (removal's legacy handover) must not take a partial + // listing for the whole; a missing ROOT is the genuine empty case. + if (options?.strict === true && !(dirRel === "" && hasErrorCode(error, "ENOENT"))) { + throw error; + } return; // Self-healing: missing/unreadable dirs list as empty. } // Iterate in path-string order — directories key as "name/" so the DFS @@ -466,8 +585,8 @@ class LocalMemoryStore implements MemoryStore { }); for (const entry of entries) { // Per-entry cap: a single flat directory can exceed the cap on its own. - if (results.length > MEMORY_MAX_FILES_PER_SCOPE) return; - if (entry.name.startsWith(".")) continue; + if (options?.strict !== true && results.length > MEMORY_MAX_FILES_PER_SCOPE) return; + if (options?.includeDotfiles !== true && entry.name.startsWith(".")) continue; const childRel = dirRel === "" ? entry.name : `${dirRel}/${entry.name}`; if (entry.isDirectory()) { await walk(childRel); @@ -480,21 +599,26 @@ class LocalMemoryStore implements MemoryStore { return results.sort(); } - async kind(relPath: string): Promise { + async kind(relPath: string, options?: { strict?: boolean }): Promise { try { const stat = await fsPromises.stat(this.abs(relPath)); return stat.isDirectory() ? "dir" : "file"; - } catch { + } catch (error) { + if (options?.strict === true && !isMissingPathError(error)) throw error; return null; } } async readFilePrefix(relPath: string, maxBytes: number): Promise { + return (await this.readFilePrefixBytes(relPath, maxBytes)).toString("utf-8"); + } + + async readFilePrefixBytes(relPath: string, maxBytes: number): Promise { const handle = await fsPromises.open(this.abs(relPath), "r"); try { const buffer = Buffer.alloc(maxBytes); const { bytesRead } = await handle.read(buffer, 0, maxBytes, 0); - return buffer.subarray(0, bytesRead).toString("utf-8"); + return buffer.subarray(0, bytesRead); } finally { await handle.close(); } @@ -752,6 +876,13 @@ export class MemoryService extends EventEmitter { */ private readonly ownerByContext = new WeakMap(); + /** + * Sub-agents whose pre-sharing private notebook was found absent or already + * adopted during this process lifetime, keyed to the owner and legacy-store + * state observed at the time (see adoptLegacyPrivateStore). + */ + private readonly legacyStoreCheckedAgainst = new Map(); + /** * Owner of the workspace scope for this context ("" when there is no * workspace). Public so callers that key sidecar metadata for the same @@ -787,7 +918,8 @@ export class MemoryService extends EventEmitter { * 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. + * owner-pinned note go. Strict sidecar read: an unreadable pin file must + * refuse, not read as "nothing pinned". */ private async assertNotPinnedForRemoval( ctx: MemoryScopeContext, @@ -798,7 +930,7 @@ export class MemoryService extends EventEmitter { const key = this.logicalKeyFor(ctx, scope, relPath); if (key === null) return; const subtreePrefix = `${key}/`; - for (const [entryKey, entry] of await this.metaService.getEntries()) { + for (const [entryKey, entry] of await this.metaService.getEntriesOrThrow()) { if (entry.pinned !== true) continue; if (entryKey === key || entryKey.startsWith(subtreePrefix)) { throw new MemoryCommandError( @@ -964,10 +1096,767 @@ export class MemoryService extends EventEmitter { /** * Every workspace-scope entry point (commands, root listing, index build) - * goes through here: refuse revoked access. + * goes through here: refuse revoked access, then fold a sub-agent's + * pre-sharing private notebook into the shared store it now resolves to. */ private async openWorkspaceStore(ctx: MemoryScopeContext, store: MemoryStore): Promise { await this.assertWorkspaceStoreReadable(ctx, store); + await this.adoptLegacyPrivateStore(ctx, store); + // The adoption pass waits for and holds the owner-store lock, a window in + // which another backend's removal can publish the acting workspace's (or + // the owner's) tombstone. The pass itself refuses on its commit guard + // and swallows that as a retryable adoption failure, so re-check here: + // the caller is about to read the owner's still-live notebook on behalf + // of a workspace that no longer exists. + await this.assertWorkspaceStoreReadable(ctx, store); + } + + /** + * Upgrade compatibility for the shared task-tree notebook. Sub-agents + * created by builds before sharing kept `/memories/workspace` in their OWN + * session dir (//memory). getStore now redirects them to + * the owner's root, which would make those notes invisible — and removal + * later deletes the child's session dir, discarding them for good. On the + * child's first shared-store access per process, copy every legacy file + * into the owner store (same relPath when free or identical; otherwise + * under imported//) and copy pins/stats to the owner key. + * + * The legacy directory is left in place, untouched: it is exactly where a + * DOWNGRADED build reads (and writes) this child's notebook, so the notes + * stay visible across upgrade↔downgrade (the child-keyed sidecar entries + * stay for the same reason) and files the import cannot carry + * (binary/oversize, doubly conflicting) are never moved anywhere. + * The copy is idempotent — identical files are skipped, differing ones land + * under imported// — so notes edited during a downgrade are folded in + * again on the next upgrade. Writes made through the shared store meanwhile + * live in the owner's notebook, which the downgraded build shows there. + * + * Security: the legacy root must be a real directory (a symlinked root + * would let an index build copy arbitrary host text into the shared + * notebook and the model's context), and every file passes the store's + * containment check before it is read. Runs under the owner store's + * mutation lock with the same commit guard as file mutations, and never + * throws: a failure (lock timeout, disk) is retried on the next access, + * while the caller proceeds with the shared store. Not journaled: this is + * a mechanical copy, not an agent edit; pre-upgrade child journal rows keep + * targeting the legacy physical paths. + */ + private async adoptLegacyPrivateStore( + ctx: MemoryScopeContext, + store: MemoryStore + ): Promise { + const childId = ctx.workspaceId; + if (childId === "") return; + const owner = this.storeOwnerWorkspaceId(store); + assert(owner !== null, "workspace-scope stores live under sessionsDir"); + if (owner === childId) { + // Not redirected: the private store IS the store. Recorded so a later + // redirect (config recovered) is seen as a change of the key below. + this.legacyStoreCheckedAgainst.set(childId, owner); + // The OWNER's access adopts its descendants' legacy notebooks too: a + // sub-agent that finished before the upgrade never touches memory + // again, and without this its notes would stay invisible to the owner + // until the child's removal hands them over. + await this.adoptDescendantLegacyStores(ctx, store, owner); + return; + } + try { + await this.adoptLegacyPrivateStoreOrThrow(ctx, store, owner); + } catch (error) { + log.warn( + "[MemoryService] failed to adopt a sub-agent's legacy workspace notebook; retrying on next access", + { + childId, + owner, + error, + } + ); + } + } + + /** + * Access-time adoption on behalf of every registered workspace resolving + * to `owner` (one config snapshot per pass). Each child costs one lstat of + * its legacy root when there is nothing to adopt — an absent root is + * skipped outright, an unchanged one is answered by the per-child memo — + * and a failing child never fails the owner's access (logged, retried on + * the next access like the child's own pass). + */ + private async adoptDescendantLegacyStores( + ctx: MemoryScopeContext, + store: MemoryStore, + owner: string + ): Promise { + const cfg = this.config.loadConfigOrDefault(); + const resolve = workspaceMemoryOwnerResolver(cfg); + for (const project of cfg.projects.values()) { + for (const workspace of project.workspaces) { + const childId = workspace.id; + if (childId === undefined || childId === owner || resolve(childId) !== owner) continue; + const legacyRoot = path.join(this.config.sessionsDir, childId, "memory"); + if ((await lstatKind(legacyRoot)) === "missing") continue; + const childCtx: MemoryScopeContext = { + runtime: null, + checkoutCwd: "", + workspaceId: childId, + projectPath: ctx.projectPath, + }; + try { + await this.adoptLegacyPrivateStoreOrThrow(childCtx, store, owner); + } catch (error) { + log.warn( + "[MemoryService] failed to adopt a sub-agent's legacy workspace notebook on the owner's behalf; retrying on next access", + { childId, owner, error } + ); + } + } + } + } + + /** + * Removal handover: before a sub-agent's session directory is deleted, + * fold its legacy private notebook (if any) into the owner store. The + * access-time adoption above only runs when some workspace-memory entry + * point serves the child; a child removed right after an upgrade (an + * inactive-descendant deletion cascade, say) may never have had one, and + * the deletion would discard its notes for good. Runs BEFORE any teardown + * step (removal reuses the owner it verified), so a failure aborts the + * removal with the workspace intact: this variant THROWS instead of + * deferring to a next access that will never come — also when a listed + * note could not be represented in the owner store (shared notebook at its + * file cap, both destinations taken by different content, unreadable as + * text): the pass would count it as skipped and the deletion would take + * the only copy. Removal runs it twice: pre-teardown, and again inside the + * removal locks (`locksHeld`, the owner-store lock among them) right before + * the tombstone, catching a note a self-fallback backend committed into the + * legacy directory in between — the child's own store lock is held there + * too, so nothing can land after that pass. + */ + async adoptLegacyPrivateStoreForRemoval( + childWorkspaceId: string, + ownerWorkspaceId: string, + options?: { locksHeld: boolean } + ): Promise { + assert(childWorkspaceId.length > 0, "adoptLegacyPrivateStoreForRemoval requires a child id"); + assert( + ownerWorkspaceId.length > 0 && ownerWorkspaceId !== childWorkspaceId, + "adoptLegacyPrivateStoreForRemoval requires a distinct owner id" + ); + // Workspace-scope keys and roots embed only the workspace id (see + // logicalKeyFor / getStore), so no project identity is needed here. + const ctx: MemoryScopeContext = { + runtime: null, + checkoutCwd: "", + workspaceId: childWorkspaceId, + projectPath: "", + }; + const store = this.getStore(ctx, "workspace"); + // The store resolved from current config must be the owner removal + // verified; a disagreement means the topology changed under removal's + // feet, and adopting into the wrong notebook would be worse than aborting. + const resolvedOwner = this.storeOwnerWorkspaceId(store); + if (resolvedOwner !== ownerWorkspaceId) { + throw new Error( + `shared memory owner of ${childWorkspaceId} resolved to ${String(resolvedOwner)} while removal verified ${ownerWorkspaceId}` + ); + } + const { skipped } = await this.adoptLegacyPrivateStoreOrThrow(ctx, store, ownerWorkspaceId, { + force: true, + locksHeld: options?.locksHeld === true, + }); + if (skipped > 0) { + throw new Error( + `${skipped} legacy workspace memory note(s) of ${childWorkspaceId} could not be folded into ${ownerWorkspaceId}'s shared notebook (full, conflicting, or not text); removing the session directory would discard them` + ); + } + } + + /** + * Fingerprint of a sub-agent's legacy private notebook as seen by the + * adoption pass: owner, legacy root kind, the legacy store's stamp (root + * entry, listed files' size/mtime) and the child-keyed sidecar entries. + * Cheap when no legacy root exists (one lstat). + */ + private async legacyAdoptionCheckKey( + childId: string, + owner: string + ): Promise<{ legacyRootKind: Awaited>; checkKey: string }> { + const childSessionDir = path.join(this.config.sessionsDir, childId); + const legacyRoot = path.join(childSessionDir, "memory"); + const legacyRootKind = await lstatKind(legacyRoot); + // Workspace-scope keys embed only the workspace id (see logicalKeyFor). + const childKeyPrefix = memoryLogicalKey("workspace", "", { + projectPath: "", + workspaceId: childId, + }); + const childSidecarFingerprint = + legacyRootKind === "dir" + ? JSON.stringify( + [...(await this.metaService.getEntries())] + .filter(([key]) => key.startsWith(childKeyPrefix)) + .sort(([a], [b]) => (a < b ? -1 : a > b ? 1 : 0)) + ) + : ""; + const legacyStamp = legacyRootKind !== "dir" ? "" : await legacyStoreStamp(legacyRoot); + const checkKey = `${owner}\u0000${legacyRootKind}\u0000${legacyStamp}\u0000${childSidecarFingerprint}`; + return { legacyRootKind, checkKey }; + } + + /** + * The adoption pass (see adoptLegacyPrivateStore). `force` skips the + * per-process "already checked" memo: removal wants the pass to run against + * the current legacy directory regardless of what an earlier access saw. + * `locksHeld`: the caller already holds the owner store's mutation lock + * (removal's in-lock delta pass), so it is not re-acquired. Returns how many + * listed legacy notes could NOT be represented in the owner store this pass. + */ + private async adoptLegacyPrivateStoreOrThrow( + ctx: MemoryScopeContext, + store: MemoryStore, + owner: string, + options?: { force: boolean; locksHeld?: boolean } + ): Promise<{ skipped: number }> { + const childId = ctx.workspaceId; + // Checked once per (child, owner, legacy-store state) per process. The + // owner is part of the key because ownership can move: a command served + // while config.json was missing/malformed resolves the child to itself + // and writes into the legacy dir. The legacy store's own state is part of + // it because ANOTHER backend can do the same while this process's + // resolution never changes: its self-fallback write replaces a root entry + // and moves the listed files' size/mtime, so either signal re-runs the + // pass. The child-keyed sidecar entries are the fourth input: a + // downgraded build can change only a pin or usage counter, which the + // manifest reconciles (sidecar fingerprint) but no file stat shows. The + // pass itself is idempotent. + const childSessionDir = path.join(this.config.sessionsDir, childId); + const legacyRoot = path.join(childSessionDir, "memory"); + const { legacyRootKind, checkKey } = await this.legacyAdoptionCheckKey(childId, owner); + if (options?.force !== true && this.legacyStoreCheckedAgainst.get(childId) === checkKey) { + return { skipped: 0 }; + } + // Not "nothing to adopt": a root that could not be inspected may hold the + // only copy of downgrade-era notes. Access-time callers log and retry; + // removal aborts with the session intact. + const unreadableRoot = (): Error => + new Error(`the legacy workspace memory root of ${childId} could not be inspected`); + if (legacyRootKind === "unreadable") throw unreadableRoot(); + if (legacyRootKind !== "dir") { + if (legacyRootKind === "symlink") { + log.warn("[MemoryService] ignoring a symlinked legacy workspace memory root", { + childId, + legacyRoot, + }); + } + this.legacyStoreCheckedAgainst.set(childId, checkKey); + return { skipped: 0 }; + } + // Files adopted this pass (bytes written OR only their sidecar entries + // folded in): either changes what the shared store's readers derive from it. + let adoptedCount = 0; + // Notes left unrepresented, split by what a retry against the SAME legacy + // store could change: permanent skips (over the cap, doubly conflicting, + // not text, escaping destination) need the legacy or owner store to + // change first; transient ones (an fs error on a read, stage, install or + // sidecar write) may clear on their own, so they keep the pass unmemoized. + let skipped = 0; + let transientSkips = 0; + const pass = async (): Promise => { + await this.assertMutationCommittable(ctx, store, undefined, toVirtualPath("workspace", "")); + const rootKindUnderLock = await lstatKind(legacyRoot); + if (rootKindUnderLock === "unreadable") throw unreadableRoot(); + if (rootKindUnderLock !== "dir") return; // swapped while waiting for the lock + const legacy = new LocalMemoryStore(legacyRoot); + // Strict: a note omitted by a partial listing would count as "nothing + // to adopt" (skipped stays 0) and removal would then delete its only + // copy. A traversal failure fails the pass instead (access-time: + // retried on the next access; removal: aborted, session intact). + // Dot-entries included: no listing shows them, but the path grammar + // admits them, so `.note` may be a real note of the downgraded child. + const files = await legacy.listFiles({ strict: true, includeDotfiles: true }); + // What was already folded in, kept in the child's session dir OUTSIDE + // the legacy root (which is a downgraded build's model-writable + // namespace; see legacyAdoptionManifestPath): per relPath the content + // hash, the fingerprint of the child-keyed sidecar entry, and where the + // copy landed. Content: without it, a note later edited through the shared + // store would be re-imported as a stale duplicate on every backend + // start. Sidecar: a downgraded build can change only a pin or usage + // stats, which must reach the owner key without the bytes changing. + // Strict reads throughout: this pass decides what the handover may + // consider done (and removal then deletes the child session on that + // basis), so a transiently unreadable manifest, sidecar or owner + // listing must fail the pass rather than stand in as "empty". + const manifestPath = legacyAdoptionManifestPath(childSessionDir); + const adopted = await this.readOrQuarantineAdoptionManifest(manifestPath, childId); + const sidecarEntries = await this.metaService.getEntriesOrThrow(); + // The per-scope file cap is a store invariant (create/rename enforce + // it): the copy stops at the owner store's remaining capacity so a + // combined notebook cannot exceed it — an over-full scope is silently + // truncated by the index and refuses every later create. Files left + // behind stay unrecorded and are retried once space frees up. Complete + // owner listing: an undercount would let the copy push the store past + // the cap and hide an adopted note's only copy once readable again. + let remainingCapacity = + MEMORY_MAX_FILES_PER_SCOPE - (await store.listFiles({ strict: true })).length; + // Staged bytes a crashed pass never installed: their records claim + // nothing (no file carries the receipt), so they are simply dropped. + const stagingDir = legacyAdoptionStagingDir(store); + await fsPromises.rm(stagingDir, { recursive: true, force: true }); + let capacityExhausted = false; + let manifestDirty = false; + let imported = 0; + const writeManifest = () => + writeFileAtomic(manifestPath, JSON.stringify(Object.fromEntries(adopted)), { + encoding: "utf-8", + }); + for (const relPath of files) { + // Same gates as a memory command. Name first: a legacy file whose + // name the path grammar rejects (traversal-looking segments, control + // characters, XML metacharacters) can never be addressed through the + // shared store, so it is never copied there — a permanent skip that + // removal reports like any other unrepresentable note. + try { + parseMemoryPath(toVirtualPath("workspace", relPath)); + } catch { + skipped++; + continue; + } + // Then containment (no symlink escape), size cap, and text-only. + // Dot-entries too (r73): `.note` is addressable, so a real note + // there may hold text `create` permitted or be transiently + // unreadable — exempting dot-entries would report a complete + // handover and let removal take the only copy. A stray `.DS_Store` + // costs a forced removal, never a note. + let bytes: Buffer; + try { + await legacy.assertContained(relPath); + bytes = await legacy.readFilePrefixBytes(relPath, MEMORY_MAX_FILE_BYTES + 1); + } catch (error) { + skipped++; + // An escaping path is permanent; a read failure (EACCES, EIO) may clear. + if (!(error instanceof MemoryCommandError)) transientSkips++; + continue; + } + if (bytes.length > MEMORY_MAX_FILE_BYTES) { + skipped++; + continue; + } + // Strict decode: invalid UTF-8 cannot be carried by a text write, but a + // note that legitimately contains U+FFFD must not be mistaken for one + // (a lossy decode would make the two indistinguishable). + let content: string; + try { + content = new TextDecoder("utf-8", { fatal: true }).decode(bytes); + } catch { + skipped++; + continue; + } + const childKey = memoryLogicalKey("workspace", relPath, { + projectPath: ctx.projectPath, + workspaceId: childId, + }); + const childEntry = sidecarEntries.get(childKey); + const record: LegacyAdoptionRecord = { + content: sha256Hex(content), + sidecar: childEntry === undefined ? "" : JSON.stringify(childEntry), + target: "", + created: false, + }; + const previous = adopted.get(relPath); + if ( + previous?.content === record.content && + previous.sidecar === record.sidecar && + previous.pending !== true + ) { + continue; // folded in earlier, nothing changed since + } + let target: { relPath: string; write: boolean } | null = null; + // A child's pin toggle folds into the copy only while the copy is + // this adoption's generation (see below) or the owner's identical + // note it was folded into at first adoption; a copy the owner + // replaced since keeps the owner's pin. + let foldChildPin = true; + if (previous !== undefined) { + // The recorded target is reused only while it still holds bytes + // this adoption put there — the owner may have edited, replaced or + // deleted it since, and the child's note must not land on unrelated + // content or a missing file. Unchanged legacy bytes (only the + // sidecar moved): reuse without a write. Otherwise the note is + // placed anew (legacyImportTarget: a legacy note edited on the + // downgraded build lands under imported// beside the copy of + // its earlier bytes). Inspected strictly: a prior target that + // merely cannot be stat'ed or read right now is not "replaced" — + // retry on the next access instead (the pass stays incomplete). + let priorContent: string | null; + try { + priorContent = await this.inspectAdoptedCopy(store, previous.target); + } catch (error) { + log.warn( + "[MemoryService] cannot inspect an adopted note's prior copy; retrying later", + { childId, owner, relPath, target: previous.target, error } + ); + skipped++; + transientSkips++; + continue; + } + if (priorContent === content) { + // Ours only while the copy is the generation this adoption + // installed (LegacyAdoptionRecord.targetStamp — a receipt taken + // on the staged bytes BEFORE they appear at the target, so even a + // pass interrupted between manifest and install left one). + // Identical bytes in another generation are the owner's: the + // owner may have deleted and recreated the note with the very + // same bytes, or — a pass interrupted before its install — + // another backend may have created it at the planned target. + // `pending` alone is never provenance: a stamp-less pending + // record (an older build's) is ambiguous and claims nothing. + const currentStamp = + (await adoptionTargetStamp(store.physicalPath(previous.target))) ?? undefined; + const ours = + previous.created === true && + currentStamp !== undefined && + currentStamp === previous.targetStamp; + target = { relPath: previous.target, write: false }; + record.created = ours; + record.targetStamp = ours ? currentStamp : undefined; + foldChildPin = !(previous.created === true && !ours); + } + } + if (target === null) { + // A destination that cannot be inspected right now (EACCES, EIO on + // its lstat or read) is neither free nor different: the note waits + // for the next pass with no copy made and no record written. + try { + target = await this.legacyImportTarget(store, childId, relPath, content); + } catch (error) { + log.warn("[MemoryService] cannot inspect a legacy note's destination; retrying later", { + childId, + owner, + relPath, + error, + }); + skipped++; + transientSkips++; + continue; + } + if (target === null) { + skipped++; + continue; + } + } + if (target.write) { + if (remainingCapacity <= 0) { + capacityExhausted = true; + skipped++; + continue; + } + // Destination containment immediately before the write (the + // same check a memory create runs): a symlinked component under + // the owner root — e.g. imported/ pointing elsewhere — + // must never let the copy land outside the shared notebook. + try { + await store.assertContained(target.relPath); + } catch (error) { + log.warn("[MemoryService] legacy note destination escapes the shared store; skipped", { + childId, + relPath, + target: target.relPath, + error, + }); + skipped++; + continue; + } + // Staged install: the bytes are written to a hidden staging entry + // of the owner store first and their identity taken there (a + // rename keeps ino, size and mtime), so the manifest can record the + // receipt of the copy BEFORE the copy appears at the target. A pass + // interrupted at any point then leaves a record that either names + // a file not yet there (nothing claimed) or names the installed + // generation by stamp; a plain byte match never has to stand in + // for provenance. Without the record, an installed copy would read + // as the owner's own note. + const stagingPath = path.join(stagingDir, randomUUID()); + try { + await fsPromises.mkdir(stagingDir, { recursive: true }); + await writeFileAtomic(stagingPath, content, { encoding: "utf-8" }); + } catch (error) { + log.warn("[MemoryService] cannot stage a legacy note for adoption; retrying later", { + childId, + relPath, + error, + }); + skipped++; + transientSkips++; + continue; + } + const stagedStamp = await adoptionTargetStamp(stagingPath); + if (stagedStamp === null) { + await fsPromises.rm(stagingPath, { force: true }); + skipped++; + transientSkips++; + continue; + } + adopted.set(relPath, { + ...record, + target: target.relPath, + created: true, + pending: true, + targetStamp: stagedStamp, + }); + await writeManifest(); + // The destination as decided above, re-checked under the lock right + // before the install: a fresh placement must still be free. + // Anything else is owner state the rename must not clobber — the + // staged bytes are dropped, the record restored, and the note is + // placed on the next pass. + const installable = (await store.kind(target.relPath, { strict: true })) === null; + const restoreRecord = async () => { + await fsPromises.rm(stagingPath, { force: true }); + if (previous === undefined) adopted.delete(relPath); + else adopted.set(relPath, previous); + await writeManifest(); + }; + if (!installable) { + await restoreRecord(); + log.warn( + "[MemoryService] adoption destination changed before install; retrying later", + { + childId, + relPath, + target: target.relPath, + } + ); + skipped++; + transientSkips++; + continue; + } + // The install: same session dir, so a plain rename (an EXDEV — the + // memory root mounted apart from its session dir — fails this note, + // not the pass). + try { + const destination = store.physicalPath(target.relPath); + await fsPromises.mkdir(path.dirname(destination), { recursive: true }); + await fsPromises.rename(stagingPath, destination); + } catch (error) { + await restoreRecord(); + log.warn("[MemoryService] cannot install a staged legacy note; retrying later", { + childId, + relPath, + target: target.relPath, + error, + }); + skipped++; + transientSkips++; + continue; + } + remainingCapacity--; + imported++; + record.created = true; + // The generation of the file just installed (see targetStamp): the + // staged receipt, unless the filesystem re-stamped the rename. + record.targetStamp = + (await adoptionTargetStamp(store.physicalPath(target.relPath))) ?? undefined; + } + record.target = target.relPath; + // Pins/stats were keyed by the child: fold them into the owner key. + // The child-keyed entry stays — like the legacy file, it is what a + // downgraded build reads. Recorded in the manifest only once this + // succeeded, so an adoption interrupted after its writeFile (or a + // failing sidecar write) retries this step on the next access. A + // first adoption keeps the owner's own pin (a note the owner tracked + // independently); a PIN the CHILD toggled since its last adoption + // (downgrade-time pin/unpin) is the newer intent and wins. Only the + // pin bit counts for that: a downgraded build merely viewing the note + // changes its usage counters, which must not drag the owner's pin + // back to the child's unchanged value. + if (childEntry !== undefined) { + // A pending record still carries the sidecar state it was recorded + // with: a fresh adoption's is the child's current state (no + // transition → first-adoption semantics). + const priorPinned = previous === undefined ? null : legacySidecarPinned(previous.sidecar); + // Only an actual boolean transition of the child's pin overrides + // the owner's; an unknown prior state never does. + const childPinChanged = priorPinned !== null && priorPinned !== childEntry.pinned; + try { + await this.metaService.mergeKeys( + childKey, + memoryLogicalKey("workspace", target.relPath, { + projectPath: ctx.projectPath, + workspaceId: owner, + }), + { pinned: childPinChanged && foldChildPin ? "source" : "target" } + ); + } catch (error) { + log.warn( + "[MemoryService] failed to fold legacy memory stats into the shared store; retrying on next access", + { relPath, error } + ); + // Counts as skipped: the note's pin/usage metadata is still + // stranded under the child key, and removal must not delete the + // child session (the only trigger for a retry) on that basis. + skipped++; + transientSkips++; + continue; + } + } + adopted.set(relPath, record); + manifestDirty = true; + adoptedCount++; + } + if (manifestDirty) await writeManifest(); + await fsPromises.rm(stagingDir, { recursive: true, force: true }); + if (capacityExhausted) { + log.warn( + "[MemoryService] shared workspace notebook is full; legacy notes left in the sub-agent's private directory until space frees up", + { childId, owner, cap: MEMORY_MAX_FILES_PER_SCOPE } + ); + } + if (adoptedCount > 0) { + log.info( + "[MemoryService] adopted a sub-agent's legacy workspace notebook into the shared store", + { childId, owner, imported, skipped } + ); + } + }; + if (options?.locksHeld === true) { + await pass(); + } else { + await withTargetMutationLock(this.config.rootDir, this.storeLockKey(store), pass); + } + // Recorded against the state observed BEFORE the pass: a foreign write + // landing during it changes the stamp and re-runs the (idempotent) pass. + // Memoized even when notes were left PERMANENTLY unrepresented (owner + // store full, both destinations taken, not text): an unchanged legacy + // store cannot adopt more on a retry, and re-walking it on every access + // would make a stuck note a per-access tax — owner-side state the key + // does not observe (freed capacity) is picked up by the next + // legacy-store change, a process restart, or removal's forced pass. A + // TRANSIENT failure (permission interval, ENOSPC, a sidecar write) may + // clear by itself, so the pass stays unmemoized and the next access + // retries it. + if (transientSkips === 0) { + this.legacyStoreCheckedAgainst.set(childId, checkKey); + } else { + this.legacyStoreCheckedAgainst.delete(childId); + } + if (adoptedCount > 0) this.emitChange(ctx, "workspace", "", "agent"); + return { skipped }; + } + + /** + * Strict manifest read that self-heals a MALFORMED file: its bytes are the + * file's state, and refusing forever would block every access-time pass + * and non-forced removal of the child. The file is quarantined beside + * itself (`.malformed-`) and the pass continues from an empty + * record map — safe because adoption is idempotent: identical files are + * skipped and differing ones land under imported//. Only the + * provenance of copies this adoption created is lost (they read as the + * owner's own from now on). An UNREADABLE manifest (EACCES, EIO) still + * fails the pass, as does a quarantine rename that fails. + */ + private async readOrQuarantineAdoptionManifest( + manifestPath: string, + childId: string + ): Promise> { + try { + return await readLegacyAdoptionManifest(manifestPath, { strict: true }); + } catch (error) { + if (!(error instanceof LegacyAdoptionManifestMalformedError)) throw error; + const quarantined = `${manifestPath}.malformed-${Date.now()}`; + try { + await fsPromises.rename(manifestPath, quarantined); + } catch (renameError) { + log.warn("[MemoryService] cannot quarantine a malformed legacy adoption manifest", { + childId, + manifestPath, + error: renameError, + }); + throw error; + } + log.warn( + "[MemoryService] quarantined a malformed legacy adoption manifest; re-adopting from scratch", + { childId, manifestPath, quarantined, error: getErrorMessage(error) } + ); + return new Map(); + } + } + + /** + * Where a legacy file lands in the owner store: its own relPath when free + * (write) or already identical (no write); the per-child import directory + * when the owner has different content there; null when even that slot is + * taken by different content (the file stays only in the legacy directory). + */ + private async legacyImportTarget( + store: MemoryStore, + childId: string, + relPath: string, + content: string + ): Promise<{ relPath: string; write: boolean } | null> { + for (const candidate of [relPath, `${LEGACY_IMPORT_DIR}/${childId}/${relPath}`]) { + // Never even compare through an escaping path (the write site re-checks). + const contained = await store.assertContained(candidate).then( + () => true, + () => false + ); + if (!contained) continue; + const destination = await this.inspectAdoptionDestination(store, candidate); + if (destination === "free") return { relPath: candidate, write: true }; + if (destination.content === content) return { relPath: candidate, write: false }; + } + return null; + } + + /** + * Content of an adopted note's copy in the owner store, or null when no + * regular listed file is there (absent, a directory, a symlink, or grown + * past the cap — each a change the owner made). Throws when the copy + * cannot be inspected at all (EACCES, EIO): callers retry later. + */ + private async inspectAdoptedCopy(store: MemoryStore, relPath: string): Promise { + const contained = await store.assertContained(relPath).then( + () => true, + () => false + ); + if (!contained) return null; + const destination = await this.inspectAdoptionDestination(store, relPath); + return destination === "free" ? null : destination.content; + } + + /** + * What an adoption destination in the owner store holds: "free" when + * nothing is there, else the text of a regular, in-cap, valid-UTF-8 file — + * or `content: null` for anything a legacy note can never equal (a + * directory, a symlink, a FIFO/socket/device, a file over the cap or not + * UTF-8), which is owner state the caller must neither read nor clobber. + * The type is settled by lstat BEFORE anything opens the entry: open() on a + * FIFO blocks until a peer shows up and would hang the pass. Destination + * bytes are decoded strictly for the same reason legacy bytes are: a lossy + * decode reads invalid UTF-8 as U+FFFD and would settle a legacy note that + * literally contains U+FFFD as "already present", leaving its only copy in + * the legacy directory. Throws when the entry cannot be inspected at all + * (EACCES, EIO): a transient failure the callers retry later, never a + * mismatch — declaring it free would clobber the owner's note, declaring it + * different would duplicate the child's under imported//. + */ + private async inspectAdoptionDestination( + store: MemoryStore, + relPath: string + ): Promise<"free" | { content: string | null }> { + let isRegularFile: boolean; + try { + isRegularFile = (await fsPromises.lstat(store.physicalPath(relPath))).isFile(); + } catch (error) { + if (isMissingPathError(error)) return "free"; + throw error; + } + if (!isRegularFile) return { content: null }; + const bytes = await store.readFilePrefixBytes(relPath, MEMORY_MAX_FILE_BYTES + 1); + if (bytes.length > MEMORY_MAX_FILE_BYTES) return { content: null }; + try { + return { content: new TextDecoder("utf-8", { fatal: true }).decode(bytes) }; + } catch { + return { content: null }; + } } /** diff --git a/src/node/services/workspaceRemoval.test.ts b/src/node/services/workspaceRemoval.test.ts index 75abc00ae0..7b1c7a8a63 100644 --- a/src/node/services/workspaceRemoval.test.ts +++ b/src/node/services/workspaceRemoval.test.ts @@ -11,10 +11,12 @@ import { import { acquireProcessFileLock, getProcessBirth } from "@/node/utils/concurrency/fileLock"; import { healRemovalTombstonesForRegisteredWorkspaces, + historyWriteLockPath, isWorkspaceRemovalTombstoned, refineApplyLockPath, REMOVAL_TOMBSTONE_HEAL_MIN_AGE_MS, removeSessionDirUnderMemoryLocks, + SharedMemoryRemovalAbortedError, rollbackRemovalTombstoneIfOwned, TombstoneNotDurableError, workspaceRemovalTombstonePath, @@ -136,6 +138,141 @@ describe("workspaceRemoval", () => { ).toBe(true); }); + test("runs beforeTombstone under the locks and aborts a shared-store removal when it throws", 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-hook"; + const childSessionDir = path.join(rootDir, "sessions", childId); + await fsPromises.mkdir(path.join(ownerSessionDir, "memory"), { recursive: true }); + await fsPromises.mkdir(childSessionDir, { recursive: true }); + + // The hook observes the locked section: the owner store lock is held, so a + // concurrent writer cannot enter while it runs. + let writerRanDuringHook = false; + let thrown: unknown; + try { + await removeSessionDirUnderMemoryLocks({ + rootDir, + sessionDir: childSessionDir, + workspaceId: childId, + attemptId: "test-attempt", + sharedWorkspaceMemorySessionDir: ownerSessionDir, + beforeTombstone: async () => { + const writer = withTargetMutationLock( + rootDir, + path.join(ownerSessionDir, "memory"), + () => { + writerRanDuringHook = true; + return Promise.resolve(); + } + ); + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(writerRanDuringHook).toBe(false); + // Let the writer settle later (after the locks release) and abort. + void writer; + throw new Error("migration failed"); + }, + }); + } catch (error) { + thrown = error; + } + expect(thrown).toBeInstanceOf(SharedMemoryRemovalAbortedError); + expect(String(thrown)).toContain("migration failed"); + expect(await isWorkspaceRemovalTombstoned(rootDir, childId)).toBe(false); + expect( + await fsPromises.access(childSessionDir).then( + () => true, + () => false + ) + ).toBe(true); + // The queued writer runs once removal released the locks. + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(writerRanDuringHook).toBe(true); + }); + + test("sub-agent removal aborts (no tombstone) when the owner store lock cannot be acquired", 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-locked"; + const childSessionDir = path.join(rootDir, "sessions", childId); + await fsPromises.mkdir(path.join(ownerSessionDir, "memory"), { recursive: true }); + await fsPromises.mkdir(childSessionDir, { recursive: true }); + + // A foreign process holds the OWNER store's cross-process lock (as in the + // r62 test): acquisition times out instead of reclaiming. + const lockPath = targetMutationLockFilePath(rootDir, path.join(ownerSessionDir, "memory")); + await fsPromises.mkdir(path.dirname(lockPath), { recursive: true }); + const birth = getProcessBirth(process.pid); + const token = + birth === null + ? `${process.pid}:feed` + : `${process.pid}:feed:${Buffer.from(birth).toString("hex")}`; + await fsPromises.writeFile(lockPath, token, { flag: "wx" }); + + let thrown: unknown; + try { + await removeSessionDirUnderMemoryLocks({ + rootDir, + sessionDir: childSessionDir, + workspaceId: childId, + attemptId: "test-attempt", + sharedWorkspaceMemorySessionDir: ownerSessionDir, + }); + } catch (error) { + thrown = error; + } + expect(thrown).toBeInstanceOf(SharedMemoryRemovalAbortedError); + // Unlike the own-store orphan path, NO tombstone is published: the + // wedged holder targets the owner's live notebook, so the child must + // stay registered and removal be retried. + expect(await isWorkspaceRemovalTombstoned(rootDir, childId)).toBe(false); + expect( + await fsPromises.access(childSessionDir).then( + () => true, + () => false + ) + ).toBe(true); + }, 20_000); + + test("sub-agent removal aborts when a failure lands after the target locks but before the tombstone", 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-history-locked"; + const childSessionDir = path.join(rootDir, "sessions", childId); + await fsPromises.mkdir(path.join(ownerSessionDir, "memory"), { recursive: true }); + await fsPromises.mkdir(childSessionDir, { recursive: true }); + + // Target locks succeed; the history write lock (taken INSIDE them) is + // held by a foreign process and times out. The old orphan path would now + // publish the tombstone with the owner-store lock already released. + const historyLock = await acquireProcessFileLock({ + lockPath: historyWriteLockPath(rootDir, childId), + timeoutMs: 1_000, + label: "history write lock (test holder)", + }); + try { + let thrown: unknown; + try { + await removeSessionDirUnderMemoryLocks({ + rootDir, + sessionDir: childSessionDir, + workspaceId: childId, + attemptId: "test-attempt", + sharedWorkspaceMemorySessionDir: ownerSessionDir, + }); + } catch (error) { + thrown = error; + } + expect(thrown).toBeInstanceOf(SharedMemoryRemovalAbortedError); + expect(await isWorkspaceRemovalTombstoned(rootDir, childId)).toBe(false); + } finally { + await historyLock[Symbol.asyncDispose](); + } + }, 30_000); + 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 1b5b06a1e5..da116567b8 100644 --- a/src/node/services/workspaceRemoval.ts +++ b/src/node/services/workspaceRemoval.ts @@ -49,6 +49,26 @@ import { acquireProcessFileLock } from "@/node/utils/concurrency/fileLock"; * ENOSPC) clears — while the workspace no longer exists anywhere else. * Keeping the workspace registered keeps removal retryable instead. */ +/** + * A sub-agent's removal failed before publishing its tombstone under its + * memory owner's store lock (lock unavailable, history lock timeout, or the + * in-lock legacy-notebook handover failing). The caller must ABORT the + * removal — no tombstone, no deregistration — because the orphan fallback + * would leave an admitted child write free to land in the owner's live + * notebook after removal, or delete the only copy of a legacy note. + */ +export class SharedMemoryRemovalAbortedError extends Error { + constructor(workspaceId: string, options?: ErrorOptions) { + super( + `Removing ${workspaceId} failed before its tombstone could be published under the shared workspace-memory store lock (${ + options?.cause instanceof Error ? options.cause.message : String(options?.cause) + }); removal aborted and can be retried`, + options + ); + this.name = "SharedMemoryRemovalAbortedError"; + } +} + export class TombstoneNotDurableError extends Error { constructor(workspaceId: string, options?: ErrorOptions) { super( @@ -141,6 +161,14 @@ export async function removeSessionDirUnderMemoryLocks(args: { * unconditional rollback rm would delete the marker the OTHER (possibly * succeeding or still-active) attempt relies on. */ + /** + * The tombstone was already published under these same locks by + * sealSubAgentForRemovalUnderMemoryLocks (sub-agents: before the checkout + * was deleted). Nothing fallible remains before the rm, and a lock failure + * here takes the orphan path instead of aborting a removal whose checkout + * is already gone. + */ + tombstoneSealed?: boolean; attemptId: string; /** * Session dir of the workspace whose `memory/` this workspace's @@ -152,6 +180,14 @@ export async function removeSessionDirUnderMemoryLocks(args: { * the mutation's tombstone check and its commit. */ sharedWorkspaceMemorySessionDir?: string; + /** + * Runs INSIDE the target locks, immediately before the tombstone is + * published: the legacy-notebook handover runs here, so a note a child + * wrote after any earlier pass (another backend) is still captured — the + * locks guarantee no further write can slip in. A throw aborts the removal + * (see SharedMemoryRemovalAbortedError). + */ + beforeTombstone?: () => Promise; }): Promise { assert(args.sessionDir.length > 0, "removeSessionDirUnderMemoryLocks requires a session dir"); // Crash clearly on a malformed config (test stubs, future refactors): an @@ -161,6 +197,111 @@ export async function removeSessionDirUnderMemoryLocks(args: { typeof args.rootDir === "string" && args.rootDir.length > 0, "removeSessionDirUnderMemoryLocks requires a rootDir" ); + assert(args.attemptId.length > 0, "removeSessionDirUnderMemoryLocks requires an attemptId"); + let tombstonePublishedUnderLocks = args.tombstoneSealed === true; + try { + await withRemovalLocks(args, async () => { + await args.beforeTombstone?.(); + // Tombstone BEFORE rm: once the locks release, any waiting writer + // re-checks it pre-commit (inside its own lock) and refuses, so the + // deleted directory cannot be recreated by a late mutation or + // journal append. A sealed tombstone is republished (idempotent): + // relying on the earlier marker alone would let a concurrent attempt's + // compensating rollback delete the only tombstone while this one + // proceeds to delete the session and deregister. + await publishRemovalTombstone(args); + tombstonePublishedUnderLocks = true; + await fsPromises.rm(args.sessionDir, { recursive: true, force: true }); + }); + } catch (error) { + // The orphan path below assumes a wedged writer's target is THIS + // workspace's retained session dir. A sub-agent's admitted memory write + // targets its OWNER's live notebook instead, so unless the tombstone was + // already published UNDER the owner-store lock, publishing it outside + // (after the lock was released, or never taken) would let a holder that + // passed its commit check — or a new writer — finish after removal. + // Abort instead: the workspace stays registered and removal is retried. + if (args.sharedWorkspaceMemorySessionDir !== undefined && !tombstonePublishedUnderLocks) { + throw new SharedMemoryRemovalAbortedError(args.workspaceId, { cause: error }); + } + // Fail-closed orphan path (r62): a wedged writer blocks the deletion, + // but the caller proceeds to deregister the workspace regardless — so + // the terminal marker must still become durable or a foreign backend + // would keep mutating memory and journaling into the retained orphan + // forever. Publishing outside the locks is safe on THIS path precisely + // because the directory is not deleted: a writer mid-commit lands in + // the orphan, and every later mutation observes the tombstone. + try { + await publishRemovalTombstone(args); + } catch (publishError) { + // No durable marker could be written at all (r63, e.g. ENOSPC): + // deregistering now would leave the orphan writable again the moment + // the transient failure clears. Signal the caller to ABORT the + // removal so the workspace stays registered and retryable. + throw new TombstoneNotDurableError(args.workspaceId, { cause: publishError }); + } + throw error; + } +} + +/** + * Sub-agent removal, BEFORE the checkout is deleted: run the final, fallible + * shared-memory handover (legacy-notebook adoption) under the full removal + * lock set and publish the removal tombstone in the same critical section. + * From here on no backend — including a downgraded one, which honors the + * same locks and tombstone at its memory commit points — can add to the + * child's notebooks, so the later session-dir deletion has nothing fallible + * left in front of it. Runs before runtime deletion so a handover failure + * aborts with the checkout intact (the caller rolls the tombstone back if + * the checkout deletion is then refused). Any failure aborts the removal + * (SharedMemoryRemovalAbortedError). + */ +export async function sealSubAgentForRemovalUnderMemoryLocks(args: { + rootDir: string; + sessionDir: string; + workspaceId: string; + attemptId: string; + sharedWorkspaceMemorySessionDir: string; + beforeTombstone: () => Promise; +}): Promise { + try { + await withRemovalLocks(args, async () => { + await args.beforeTombstone(); + await publishRemovalTombstone(args); + }); + } catch (error) { + throw new SharedMemoryRemovalAbortedError(args.workspaceId, { cause: error }); + } +} + +async function publishRemovalTombstone(args: { + rootDir: string; + workspaceId: string; + attemptId: string; +}): Promise { + assert(args.attemptId.length > 0, "removal tombstone requires an attemptId"); + const tombstonePath = workspaceRemovalTombstonePath(args.rootDir, args.workspaceId); + await fsPromises.mkdir(path.dirname(tombstonePath), { recursive: true }); + await writeFileAtomic( + tombstonePath, + JSON.stringify({ + workspaceId: args.workspaceId, + removedAt: Date.now(), + attemptId: args.attemptId, + }) + ); +} + +/** The removal critical section's lock set (see removeSessionDirUnderMemoryLocks). */ +async function withRemovalLocks( + args: { + rootDir: string; + sessionDir: string; + workspaceId: string; + sharedWorkspaceMemorySessionDir?: string; + }, + body: () => Promise +): Promise { // Same key derivations as MemoryService.storeLockKey: the workspace store // root lives inside the session directory; global/project mutations hold // the coarse `/memory` key while journaling into this session dir. @@ -182,20 +323,7 @@ export async function removeSessionDirUnderMemoryLocks(args: { // sidecar writers (headless usage) serialize their tombstone check + // commit against this same key, closing their check→write window. const sessionDirKey = path.resolve(args.sessionDir); - assert(args.attemptId.length > 0, "removeSessionDirUnderMemoryLocks requires an attemptId"); - const publishTombstone = async (): Promise => { - const tombstonePath = workspaceRemovalTombstonePath(args.rootDir, args.workspaceId); - await fsPromises.mkdir(path.dirname(tombstonePath), { recursive: true }); - await writeFileAtomic( - tombstonePath, - JSON.stringify({ - workspaceId: args.workspaceId, - removedAt: Date.now(), - attemptId: args.attemptId, - }) - ); - }; - try { + { // Refine serialization (r66) — acquired FIRST (r67): a /refine apply in // ANOTHER backend is untouched by the remover's process-local // cancellation and holds this same (session-dir-external) lock across @@ -231,32 +359,9 @@ export async function removeSessionDirUnderMemoryLocks(args: { timeoutMs: 10_000, label: "history write lock (removal)", }); - // Tombstone BEFORE rm: once the locks release, any waiting writer - // re-checks it pre-commit (inside its own lock) and refuses, so the - // deleted directory cannot be recreated by a late mutation or - // journal append. - await publishTombstone(); - await fsPromises.rm(args.sessionDir, { recursive: true, force: true }); + await body(); } ); - } catch (error) { - // Fail-closed orphan path (r62): a wedged writer blocks the deletion, - // but the caller proceeds to deregister the workspace regardless — so - // the terminal marker must still become durable or a foreign backend - // would keep mutating memory and journaling into the retained orphan - // forever. Publishing outside the locks is safe on THIS path precisely - // because the directory is not deleted: a writer mid-commit lands in - // the orphan, and every later mutation observes the tombstone. - try { - await publishTombstone(); - } catch (publishError) { - // No durable marker could be written at all (r63, e.g. ENOSPC): - // deregistering now would leave the orphan writable again the moment - // the transient failure clears. Signal the caller to ABORT the - // removal so the workspace stays registered and retryable. - throw new TombstoneNotDurableError(args.workspaceId, { cause: publishError }); - } - throw error; } } diff --git a/src/node/services/workspaceService.test.ts b/src/node/services/workspaceService.test.ts index 67270f9fb3..4c9fb9f383 100644 --- a/src/node/services/workspaceService.test.ts +++ b/src/node/services/workspaceService.test.ts @@ -59,6 +59,9 @@ import type { } from "@/common/types/workspace"; import { makeAgentTaskIntegrationFake } from "./taskWorkspaceSeam.testUtils"; import { resolveWorkspaceMemoryOwnerId } from "./memoryWorkspaceOwner"; +import { isWorkspaceRemovalTombstoned } from "./workspaceRemoval"; +import { MemoryService } from "./memoryService"; +import { MemoryMetaService } from "./memoryMeta"; import type { BackgroundProcessManager } from "./backgroundProcessManager"; import type { TerminalService } from "@/node/services/terminalService"; import type { DesktopSessionManager } from "@/node/services/desktop/DesktopSessionManager"; @@ -14676,6 +14679,250 @@ describe("WorkspaceService remove timing rollup", () => { }); }); +describe("WorkspaceService remove sub-agent handover ordering", () => { + // A sub-agent's final shared-memory handover + removal tombstone are sealed + // under the removal locks BEFORE the checkout is deleted: a handover the + // owner store cannot take aborts with the checkout intact, and a refused + // checkout deletion rolls the tombstone back. + const projectPath = "/tmp/proj-handover"; + const workspaceId = "child-handover"; + const ownerId = "owner-handover"; + const workspacePath = path.join(projectPath, "child-ws"); + const runtimeConfig = { type: "worktree" as const, srcBaseDir: "/tmp/src" }; + let rootDir: string; + + beforeEach(async () => { + rootDir = path.join(tmpdir(), "mux-handover-order", `root-${crypto.randomUUID()}`); + await fsPromises.mkdir(path.join(rootDir, "sessions", workspaceId), { recursive: true }); + }); + afterEach(async () => { + await fsPromises.rm(rootDir, { recursive: true, force: true }); + }); + + function buildConfig(): Partial { + const topology = { + projects: new Map([ + [ + projectPath, + { + trusted: true, + workspaces: [ + { + id: ownerId, + name: "owner", + path: path.join(projectPath, "owner-ws"), + runtimeConfig, + }, + { + id: workspaceId, + name: "child", + path: workspacePath, + runtimeConfig, + parentWorkspaceId: ownerId, + }, + ], + }, + ], + ]), + }; + return { + rootDir, + srcDir: "/tmp/src", + sessionsDir: path.join(rootDir, "sessions"), + removeWorkspace: mock(() => Promise.resolve()), + findWorkspace: mock(() => ({ workspacePath, projectPath })), + loadConfigOrDefault: mock(() => topology), + editConfig: mock((edit: (cfg: typeof topology) => typeof topology) => + Promise.resolve(edit(topology)) + ), + } as unknown as Partial; + } + + function buildAiService(): AIService { + return { + ...createStreamLifecycleMocks(), + isStreaming: mock(() => false), + stopStream: mock(() => Promise.resolve(Ok(undefined))), + getWorkspaceMetadata: mock(() => + Promise.resolve( + Ok({ + id: workspaceId, + name: "child", + projectPath, + projectName: "proj", + runtimeConfig, + parentWorkspaceId: ownerId, + }) + ) + ), + on: mock(() => undefined), + off: mock(() => undefined), + } as unknown as AIService; + } + + test("a handover the owner cannot take aborts before the checkout is deleted", async () => { + const deleteWorkspace = mock(() => + Promise.resolve({ success: true as const, deletedPath: workspacePath }) + ); + const createRuntimeSpy = spyOn(runtimeFactory, "createRuntime").mockReturnValue({ + deleteWorkspace, + } as unknown as ReturnType); + try { + const workspaceService = createWorkspaceServiceForTest({ + config: buildConfig(), + aiService: buildAiService(), + }); + let adoptions = 0; + workspaceService.setSharedWorkspaceMemoryStore({ + adoptLegacyPrivateStoreForRemoval: (_child, _owner, options) => { + adoptions++; + // The unlocked pre-pass succeeds; the late note appears for the + // locked pass, which cannot place it. + return options?.locksHeld + ? Promise.reject(new Error("1 legacy note could not be folded")) + : Promise.resolve(); + }, + }); + const result = await workspaceService.remove(workspaceId); + expect(result.success).toBe(false); + if (!result.success) expect(result.error).toContain("could not be folded"); + expect(adoptions).toBe(2); + expect(deleteWorkspace).not.toHaveBeenCalled(); + expect(existsSync(path.join(rootDir, "sessions", workspaceId))).toBe(true); + expect(await isWorkspaceRemovalTombstoned(rootDir, workspaceId)).toBe(false); + // force accepts the loss and completes the removal. + expect((await workspaceService.remove(workspaceId, true)).success).toBe(true); + expect(deleteWorkspace).toHaveBeenCalledTimes(1); + expect(existsSync(path.join(rootDir, "sessions", workspaceId))).toBe(false); + } finally { + createRuntimeSpy.mockRestore(); + } + }); + + test("a teardown step failing after the seal rolls the tombstone back and releases the gate", async () => { + const deleteWorkspace = mock(() => + Promise.resolve({ success: true as const, deletedPath: workspacePath }) + ); + const createRuntimeSpy = spyOn(runtimeFactory, "createRuntime").mockReturnValue({ + deleteWorkspace, + } as unknown as ReturnType); + try { + const config = buildConfig(); + const workspaceService = createWorkspaceServiceForTest({ + config, + aiService: buildAiService(), + }); + workspaceService.setSharedWorkspaceMemoryStore({ + adoptLegacyPrivateStoreForRemoval: () => Promise.resolve(), + }); + // The consolidation drain runs once before the seal and again after the + // checkout deletion; the second call stands in for any teardown step + // that rejects once the child is durably tombstoned. + const calls: string[] = []; + let cancels = 0; + let failSecondCancel = true; + workspaceService.setMemoryConsolidationService({ + triggerInBackground: () => undefined, + triggerHarvestThenSweepInBackground: () => undefined, + cancelInFlightConsolidation: () => { + calls.push("cancel"); + cancels++; + return cancels === 2 && failSecondCancel + ? Promise.reject(new Error("sandbox teardown failed")) + : Promise.resolve(); + }, + releaseRemovalCancellation: () => { + calls.push("release"); + }, + finalizeHarvestsForRemoval: () => { + calls.push("finalize"); + return Promise.resolve(); + }, + }); + const failed = await workspaceService.remove(workspaceId); + expect(failed.success).toBe(false); + if (!failed.success) expect(failed.error).toContain("sandbox teardown failed"); + // Still registered: the sealed marker is gone, the gate lifted, and + // nothing was finalized. + expect(config.removeWorkspace).not.toHaveBeenCalled(); + expect(await isWorkspaceRemovalTombstoned(rootDir, workspaceId)).toBe(false); + expect(existsSync(path.join(rootDir, "sessions", workspaceId))).toBe(true); + expect(calls).toContain("release"); + expect(calls).not.toContain("finalize"); + // The child's memory works again: its shared-store write is not + // refused by a stale tombstone. + const memoryService = new MemoryService( + { + rootDir, + sessionsDir: path.join(rootDir, "sessions"), + loadConfigOrDefault: config.loadConfigOrDefault, + configFileStamp: () => "stable", + onConfigChanged: () => undefined, + } as unknown as Config, + new MemoryMetaService(rootDir) + ); + const created = await memoryService.create( + { runtime: null, checkoutCwd: "", workspaceId, projectPath: "" }, + "/memories/workspace/after-abort.md", + "still usable", + "agent" + ); + expect(created.success).toBe(true); + // A retried removal completes: cancelled, tombstoned, finalized, not released. + failSecondCancel = false; + calls.length = 0; + expect((await workspaceService.remove(workspaceId)).success).toBe(true); + expect(await isWorkspaceRemovalTombstoned(rootDir, workspaceId)).toBe(true); + expect(calls).toContain("finalize"); + expect(calls).not.toContain("release"); + } finally { + createRuntimeSpy.mockRestore(); + } + }); + + test("a refused checkout deletion rolls the sealed tombstone back", async () => { + let refuse = true; + const deleteWorkspace = mock(() => + Promise.resolve( + refuse + ? { success: false as const, error: "Workspace has uncommitted changes" } + : { success: true as const, deletedPath: workspacePath } + ) + ); + const createRuntimeSpy = spyOn(runtimeFactory, "createRuntime").mockReturnValue({ + deleteWorkspace, + } as unknown as ReturnType); + try { + const workspaceService = createWorkspaceServiceForTest({ + config: buildConfig(), + aiService: buildAiService(), + }); + let sealedTombstone = false; + workspaceService.setSharedWorkspaceMemoryStore({ + adoptLegacyPrivateStoreForRemoval: () => Promise.resolve(), + }); + deleteWorkspace.mockImplementation(async () => { + // Runtime deletion runs with the tombstone already sealed. + sealedTombstone = await isWorkspaceRemovalTombstoned(rootDir, workspaceId); + return refuse + ? { success: false as const, error: "Workspace has uncommitted changes" } + : { success: true as const, deletedPath: workspacePath }; + }); + const refused = await workspaceService.remove(workspaceId); + expect(refused.success).toBe(false); + if (!refused.success) expect(refused.error).toContain("uncommitted changes"); + expect(sealedTombstone).toBe(true); + expect(await isWorkspaceRemovalTombstoned(rootDir, workspaceId)).toBe(false); + expect(existsSync(path.join(rootDir, "sessions", workspaceId))).toBe(true); + refuse = false; + expect((await workspaceService.remove(workspaceId)).success).toBe(true); + expect(await isWorkspaceRemovalTombstoned(rootDir, workspaceId)).toBe(true); + } finally { + createRuntimeSpy.mockRestore(); + } + }); +}); + describe("WorkspaceService remove shared-workspace guard", () => { const projectPath = "/tmp/proj-shared"; const workspaceId = "child-shared"; @@ -15159,12 +15406,51 @@ describe("WorkspaceService remove desktop session cleanup", () => { const aborted = await workspaceService.remove(workspaceId); expect(aborted.success).toBe(false); expect(calls).toEqual(["release"]); - // Committed removal: cancelled (drained), harvest records finalized once - // the session directory is gone, and never released. + // Aborted inside the locked handover (a late legacy note the owner store + // cannot take), i.e. after the drain but BEFORE the tombstone: the session + // directory survives, so the gate is lifted too. descendants = false; calls.length = 0; const sessionDir = path.join(tempRoot, "sessions", workspaceId); await fsPromises.mkdir(sessionDir, { recursive: true }); + const topology = { + projects: new Map([ + [ + "/tmp/src/project", + { + workspaces: [ + { path: "/tmp/src/project/owner", id: "ws-owner" }, + { path: "/tmp/src/project/child", id: workspaceId, parentWorkspaceId: "ws-owner" }, + ], + }, + ], + ]), + }; + // The service holds its own copy of the mock config (createWorkspaceServiceForTest). + const config = (workspaceService as unknown as { config: MockWorkspaceConfig }).config; + const previousLoad = config.loadConfigOrDefault; + config.loadConfigOrDefault = (() => topology) as MockWorkspaceConfig["loadConfigOrDefault"]; + workspaceService.setSharedWorkspaceMemoryStore({ + adoptLegacyPrivateStoreForRemoval: () => + Promise.reject(new Error("1 legacy note could not be folded into the shared notebook")), + }); + try { + const lockedAbort = await workspaceService.remove(workspaceId); + expect(lockedAbort.success).toBe(false); + if (!lockedAbort.success) expect(lockedAbort.error).toContain("tombstone could be published"); + expect(existsSync(sessionDir)).toBe(true); + expect(calls).toContain("cancel"); + expect(calls).toContain("release"); + expect(calls).not.toContain("finalize"); + } finally { + config.loadConfigOrDefault = previousLoad; + workspaceService.setSharedWorkspaceMemoryStore({ + adoptLegacyPrivateStoreForRemoval: () => Promise.resolve(), + }); + } + // Committed removal: cancelled (drained), harvest records finalized once + // the session directory is gone, and never released. + calls.length = 0; const removed = await workspaceService.remove(workspaceId); expect(removed.success).toBe(true); expect(existsSync(sessionDir)).toBe(false); @@ -15173,6 +15459,28 @@ describe("WorkspaceService remove desktop session cleanup", () => { expect(calls).not.toContain("release"); }); + test("remove() succeeds and emits its removal event when finalizing harvest records fails", async () => { + workspaceService.setMemoryConsolidationService({ + triggerInBackground: () => undefined, + triggerHarvestThenSweepInBackground: () => undefined, + cancelInFlightConsolidation: () => Promise.resolve(), + releaseRemovalCancellation: () => undefined, + finalizeHarvestsForRemoval: () => Promise.reject(new Error("sidecar unwritable")), + }); + const removed: string[] = []; + workspaceService.on("metadata", (event: { workspaceId: string; metadata: unknown }) => { + if (event.metadata === null) removed.push(event.workspaceId); + }); + const sessionDir = path.join(tempRoot, "sessions", workspaceId); + await fsPromises.mkdir(sessionDir, { recursive: true }); + // Deregistration already committed: harvest bookkeeping is best-effort. + const result = await workspaceService.remove(workspaceId); + expect(result.success).toBe(true); + expect(removeWorkspaceMock).toHaveBeenCalledWith(workspaceId); + expect(removed).toEqual([workspaceId]); + expect(existsSync(sessionDir)).toBe(false); + }); + test("remove() flushes the timeline before deleting the session directory", async () => { const sessionDir = path.join(tempRoot, "sessions", workspaceId); await fsPromises.mkdir(sessionDir, { recursive: true }); diff --git a/src/node/services/workspaceService.ts b/src/node/services/workspaceService.ts index fa478f984c..c4dfb5fa6a 100644 --- a/src/node/services/workspaceService.ts +++ b/src/node/services/workspaceService.ts @@ -134,9 +134,17 @@ import { pinDescendantWorkspaceMemoryOwners, resolveWorkspaceMemoryOwnerId, } from "@/node/services/memoryWorkspaceOwner"; +import type { MemoryService } from "@/node/services/memoryService"; +/** Narrow MemoryService surface removal needs for the shared-memory handover. */ +type SharedWorkspaceMemoryStoreForRemoval = Pick< + MemoryService, + "adoptLegacyPrivateStoreForRemoval" +>; import { healRemovalTombstonesForRegisteredWorkspaces, removeSessionDirUnderMemoryLocks, + sealSubAgentForRemovalUnderMemoryLocks, + SharedMemoryRemovalAbortedError, refineApplyLockPath, rollbackRemovalTombstoneIfOwned, startRemovalTombstoneLease, @@ -2749,6 +2757,8 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { releaseRemovalCancellation(workspaceId: string): void; finalizeHarvestsForRemoval(workspaceId: string): Promise; }; + /** Narrow MemoryService surface for removal's shared-memory handover; wired by coreServices. */ + private sharedWorkspaceMemoryStore?: SharedWorkspaceMemoryStoreForRemoval; private worktreeArchiveSnapshotService?: WorktreeArchiveSnapshotLifecycleService; private agentTaskIntegration?: AgentTaskIntegration; private workspaceGoalService?: WorkspaceGoalService; @@ -3097,6 +3107,10 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { this.memoryConsolidationService = service; } + setSharedWorkspaceMemoryStore(store: SharedWorkspaceMemoryStoreForRemoval): void { + this.sharedWorkspaceMemoryStore = store; + } + setWorkspaceLifecycleHooks(hooks: WorkspaceLifecycleHooks): void { this.workspaceLifecycleHooks = hooks; } @@ -4223,6 +4237,33 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { } } + /** + * Removal's in-lock shared-memory handover (see removeSessionDirUnderMemoryLocks + * `beforeTombstone`): the legacy-notebook adoption delta pass, run while + * the owner-store lock is held so nothing can land after it. Throws to + * abort the removal unless `force` accepts the loss. + */ + private async lockedSharedMemoryHandover( + workspaceId: string, + ownerWorkspaceId: string, + force: boolean + ): Promise { + try { + await this.sharedWorkspaceMemoryStore?.adoptLegacyPrivateStoreForRemoval( + workspaceId, + ownerWorkspaceId, + { locksHeld: true } + ); + } catch (error) { + if (!force) throw error; + log.warn("Forced removal: locked shared-memory handover to the owner failed", { + workspaceId, + ownerWorkspaceId, + error: getErrorMessage(error), + }); + } + } + /** Transfer destructive cleanup out of a callback that still owns a session lease. */ deferWorkspaceCleanup(run: () => Promise): void { this.trackWorkspaceCleanup(run).catch((error: unknown) => @@ -5814,11 +5855,22 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { this.removingWorkspaces.add(workspaceId); let timelineClosed = false; let removedFromConfig = false; - // Set once removal passes its point of no return (session teardown and - // tombstone follow unconditionally); an abort before that leaves the - // workspace registered and intact, so the finally lifts the consolidation - // teardown gate the drains below installed. - let removalCommitted = false; + // Set once this attempt published the durable removal tombstone (sealed + // sub-agent handover, or the session-dir teardown). If the removal then + // ends with the workspace STILL REGISTERED — a refused checkout deletion, + // a later teardown step throwing, deregistration failing — the marker is + // rolled back in the finally (ownership-checked, r66): left in place it + // would refuse every later memory access and removal retry of a + // workspace that still exists. Only a completed deregistration keeps it. + let tombstonePublished = false; + // r66: identifies THIS removal attempt in the durable tombstone so the + // compensating rollback below cannot delete a concurrent backend + // attempt's marker. + const removalAttemptId = crypto.randomUUID(); + // Sub-agents: the tombstone was published (with the final shared-memory + // handover) BEFORE the checkout deletion; an abort between the two rolls + // it back so the intact workspace stays usable. + let sealedForRemoval = false; // If this workspace is mid-init, cancel the fire-and-forget init work (postCreateSetup, // sync/checkout, .xum/init hook, etc.) so removal doesn't leave orphaned background work. @@ -5856,6 +5908,17 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { if (this.agentTaskIntegration?.hasDescendantAgentTasks(workspaceId) === true) { return Err(DESCENDANT_WORKSPACE_REMOVE_ERROR); } + // r65: keep renewing the removal tombstone's mtime until this removal + // settles so a foreign backend's startup self-heal cannot mistake a + // merely SLOW removal (a hung runtime deletion or MCP server close) for + // crash residue and delete the marker while removal is live — a healed + // marker would readmit child writes after the final shared-memory + // handover (sealSubAgentForRemovalUnderMemoryLocks). Held from before the + // earliest publish point: ticks against a not-yet-published marker are + // swallowed ENOENTs, as are ticks after a rollback deleted it, and + // disposal at scope exit (after deregistration or its rollback) is safe + // since a late renewal of a retained terminal marker is meaningless. + using _tombstoneLease = startRemovalTombstoneLease(this.config.rootDir, workspaceId); // Forced removals too (routine task cleanup uses force): proceeding // while a stalled writer still owns the lock would let it resume after // the deletion and recreate the removed path. The acquisition is @@ -5893,6 +5956,7 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { // Raw terminal listeners may enqueue timing writes; join those before rollup/removal. await this.sessionTimingService?.waitForIdle(workspaceId); + const sessionDir = path.join(this.config.sessionsDir, workspaceId); let parentWorkspaceId: string | null = null; // Memory owner resolved while the workspace was still fully registered @@ -5969,10 +6033,15 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { // Shared workspace memory (sub-agents write into their task-tree // owner's store). BEFORE any destructive step — so a failure leaves a - // fully intact, retryable workspace: pin the owner on surviving - // descendants (their parent chain is about to lose this node), - // verified by reading the config back because Config swallows write - // failures. + // fully intact, retryable workspace: + // - pin the owner on surviving descendants (their parent chain is + // about to lose this node), verified by reading the config back + // because Config swallows write failures; + // - fold this workspace's pre-sharing private notebook into the + // owner's store. A second, delta pass runs under the removal locks + // below so a note that lands in between is captured too; that late + // pass only has the few notes written since this one, keeping the + // fallible work at the point of no return minimal. const sharedMemoryOwnerId = resolveWorkspaceMemoryOwnerId( this.config.loadConfigOrDefault(), workspaceId @@ -5992,6 +6061,14 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { throw new Error(`memory owner pin for descendant ${id} did not persist`); } } + // A pre-sharing build kept this child's notebook in its OWN + // session dir (//memory); access-time adoption + // may never have run for a child removed right after the upgrade, + // and the deletion below would take those notes with it. + await this.sharedWorkspaceMemoryStore?.adoptLegacyPrivateStoreForRemoval( + workspaceId, + sharedMemoryOwnerId + ); } catch (error) { if (!force) { return Err( @@ -6004,6 +6081,27 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { error: getErrorMessage(error), }); } + // Final handover + tombstone under the removal locks, BEFORE the + // checkout is deleted (sealSubAgentForRemovalUnderMemoryLocks): a + // late legacy note the owner store cannot take must abort while the + // checkout still exists, and once sealed no backend can add + // another (they honor the tombstone at their commit points), so the + // session-dir deletion after runtime deletion has nothing fallible + // left. `force` accepts the loss of notes the handover cannot place. + await sealSubAgentForRemovalUnderMemoryLocks({ + rootDir: this.config.rootDir, + sessionDir, + workspaceId, + attemptId: removalAttemptId, + sharedWorkspaceMemorySessionDir: path.join( + this.config.sessionsDir, + sharedMemoryOwnerId + ), + beforeTombstone: () => + this.lockedSharedMemoryHandover(workspaceId, sharedMemoryOwnerId, force), + }); + sealedForRemoval = true; + tombstonePublished = true; } if (isMultiProject(metadata)) { @@ -6301,7 +6399,6 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { // // Intentionally deferred until we're committed to removal: if runtime deletion fails with // force=false we return early and keep init state intact so init-end can refresh metadata. - removalCommitted = true; this.initStateManager.clearInMemoryState(workspaceId); // Dispose the session before deleting its directory: disposal aborts the active stream, and @@ -6352,11 +6449,6 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { ); // Remove session data - const sessionDir = path.join(this.config.sessionsDir, workspaceId); - // r66: identifies THIS removal attempt in the durable tombstone so the - // compensating rollback below cannot delete a concurrent backend - // attempt's marker. - const removalAttemptId = crypto.randomUUID(); try { if (parentWorkspaceId) { try { @@ -6393,45 +6485,48 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { const memoryOwnerId = verifiedSharedMemoryOwnerId ?? resolveWorkspaceMemoryOwnerId(this.config.loadConfigOrDefault(), workspaceId); + const ownerSessionDir = + memoryOwnerId === workspaceId + ? undefined + : path.join(this.config.sessionsDir, memoryOwnerId); await removeSessionDirUnderMemoryLocks({ rootDir: this.config.rootDir, sessionDir, workspaceId, attemptId: removalAttemptId, - sharedWorkspaceMemorySessionDir: - memoryOwnerId === workspaceId + sharedWorkspaceMemorySessionDir: ownerSessionDir, + tombstoneSealed: sealedForRemoval, + // Sealed above (metadata path): handover done and tombstone + // published under these locks already. Otherwise (phantom, + // metadata-less path) the handover runs here, inside the locks + // and right before the tombstone. Throws → removal aborts, + // session intact. + beforeTombstone: + ownerSessionDir === undefined || sealedForRemoval ? undefined - : path.join(this.config.sessionsDir, memoryOwnerId), + : () => this.lockedSharedMemoryHandover(workspaceId, memoryOwnerId, force), }); - // Only once the session (and with it the transcript) is gone are the - // retryable harvest records truly unrecoverable; an aborted removal - // above must leave them retryable. - await this.memoryConsolidationService?.finalizeHarvestsForRemoval(workspaceId); + tombstonePublished = true; } catch (error) { // r63: without a durable tombstone the retained orphan stays // writable by foreign backends forever — abort the removal (the // workspace stays registered and retryable) instead of proceeding // to deregistration below. - if (error instanceof TombstoneNotDurableError) { - // No durable tombstone was published: the workspace stays - // registered with its session directory intact, so the - // consolidation teardown gate is lifted again in the finally like - // any pre-commit abort. - removalCommitted = false; + if ( + error instanceof TombstoneNotDurableError || + error instanceof SharedMemoryRemovalAbortedError + ) { + // No durable tombstone was published (the locked handover or the + // tombstone write itself failed): the workspace stays registered + // with its session directory intact, so the consolidation teardown + // gate is lifted again in the finally like any pre-commit abort. throw error; } + // Orphan path (r62): the directory was retained but the tombstone + // is durable, and deregistration proceeds below. + tombstonePublished = true; log.error(`Failed to remove session directory for ${workspaceId}:`, error); } - // r65: the tombstone is durable here (both the locked path and the - // orphan fallback published it). Keep renewing its mtime until this - // removal settles so a foreign backend's startup self-heal cannot - // mistake a merely SLOW removal (e.g. a hung MCP server close below) - // for crash residue and delete the marker while removal is live. - // Disposal at scope exit (after deregistration or its rollback) is - // safe: a late renewal of a retained terminal marker is meaningless, - // and utimes on a rolled-back (deleted) marker is a swallowed ENOENT. - using _tombstoneLease = startRemovalTombstoneLease(this.config.rootDir, workspaceId); - // The on-disk devtools.jsonl died with the session directory above; also drop any // in-memory DevTools state so stale runs cannot outlive the workspace. try { @@ -6511,6 +6606,19 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { } removedFromConfig = true; this.autoTitlingWorkspaces.delete(workspaceId); + // Only once the workspace is deregistered (and its session, with the + // transcript, gone) are the retryable harvest records truly + // unrecoverable; an aborted removal must leave them retryable. + // Best-effort: the removal is committed, so a sidecar failure here + // must not turn it into an error (the metadata event below still fires). + try { + await this.memoryConsolidationService?.finalizeHarvestsForRemoval(workspaceId); + } catch (error) { + log.warn("Failed to finalize harvest records after workspace removal", { + workspaceId, + error: getErrorMessage(error), + }); + } // Deregistration succeeded: drop the workspace's activity/status entry // so extensionMetadata.json stays bounded (stale entries were @@ -6543,7 +6651,28 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { const message = getErrorMessage(error); return Err(`Failed to remove workspace: ${message}`); } finally { - if (!removalCommitted) { + if (!removedFromConfig) { + // The workspace is still registered (a refused checkout deletion, a + // teardown step that threw, deregistration that failed): lift this + // attempt's tombstone again (ownership-checked, r66) so it stays + // usable, and the consolidation teardown gate with it. + if (tombstonePublished) { + try { + await rollbackRemovalTombstoneIfOwned({ + rootDir: this.config.rootDir, + sessionDir: path.join(this.config.sessionsDir, workspaceId), + workspaceId, + attemptId: removalAttemptId, + workspaceStillRegistered: () => this.config.findWorkspace(workspaceId) != null, + }); + } catch (rollbackError) { + log.error( + "Failed to roll back the removal tombstone after an aborted removal; " + + "the startup self-heal will reclaim it", + { workspaceId, rollbackError } + ); + } + } this.memoryConsolidationService?.releaseRemovalCancellation(workspaceId); } if (releaseOverridesLock !== undefined) {