From a5bbc6b619f73d8b2b32798f64861d48305d1b72 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 13:44:02 +0000 Subject: [PATCH 1/9] =?UTF-8?q?=F0=9F=A4=96=20feat:=20fold=20memory=20side?= =?UTF-8?q?car=20entries=20into=20a=20second=20key=20and=20refuse=20healed?= =?UTF-8?q?-substitute=20mutations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mergeKeys copies a subtree's pins/usage onto a second logical key (keeping the source; larger counters win, the pin stays with the target unless the caller says the source's is newer). loadWithHealth distinguishes a sidecar that exists but could not be read from a missing one: reads still heal to empty, but that view is neither cached nor written back by a mutation, and getEntriesOrThrow refuses it for callers that consume the entries destructively. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$34.37`_ --- src/node/services/memoryMeta.test.ts | 98 ++++++++++++++++++++++- src/node/services/memoryMeta.ts | 115 +++++++++++++++++++++++++-- 2 files changed, 203 insertions(+), 10 deletions(-) diff --git a/src/node/services/memoryMeta.test.ts b/src/node/services/memoryMeta.test.ts index b39581fb4b8..db225a7b9ff 100644 --- a/src/node/services/memoryMeta.test.ts +++ b/src/node/services/memoryMeta.test.ts @@ -1,9 +1,9 @@ -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"; describe("memoryLogicalKey", () => { @@ -249,4 +249,98 @@ 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 { + await expect(strict.getEntriesOrThrow()).rejects.toThrow(/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 137e650e6c3..5e2c086301c 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. From 79ef1f09e3d621c762899b3304fc00285ffc9f7c Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 13:44:02 +0000 Subject: [PATCH 2/9] =?UTF-8?q?=F0=9F=A4=96=20feat:=20adopt=20a=20sub-agen?= =?UTF-8?q?t's=20pre-sharing=20private=20notebook=20into=20the=20shared=20?= =?UTF-8?q?store?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sub-agents created before sharing kept /memories/workspace under their own session dir; the owner redirect made those notes invisible and removal would delete them. On the child's first shared-store access per process (and again, strictly, before removal deletes its session dir) every legacy file is copied into the owner's store — same relPath when free or identical, imported// on a conflict — via a staged rename under the owner store lock, with child-keyed pins/usage folded onto the owner key and a manifest (beside the legacy root, never inside it) recording what landed where. The legacy directory is left untouched for downgraded builds; identical files are skipped, notes edited on a downgraded build re-adopt under imported/. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$34.37`_ --- src/node/services/memoryLegacyAdoption.ts | 161 +++++ src/node/services/memoryService.test.ts | 503 +++++++++++++++ src/node/services/memoryService.ts | 754 +++++++++++++++++++++- 3 files changed, 1404 insertions(+), 14 deletions(-) create mode 100644 src/node/services/memoryLegacyAdoption.ts diff --git a/src/node/services/memoryLegacyAdoption.ts b/src/node/services/memoryLegacyAdoption.ts new file mode 100644 index 00000000000..d2d3353e6b5 --- /dev/null +++ b/src/node/services/memoryLegacyAdoption.ts @@ -0,0 +1,161 @@ +/** + * 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, + }; +} + +/** + * 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 Error(`the legacy adoption manifest at ${manifestPath} is malformed (${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/memoryService.test.ts b/src/node/services/memoryService.test.ts index 5c9b3b85f15..d73f7366c63 100644 --- a/src/node/services/memoryService.test.ts +++ b/src/node/services/memoryService.test.ts @@ -26,6 +26,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 +1489,508 @@ 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. + const ownerCtx = { ...fixture.ctx, workspaceId: "ws-owner" }; + await fixture.service.create( + ownerCtx, + "/memories/workspace/sub/same.md", + "identical", + "agent" + ); + await fixture.service.create( + ownerCtx, + "/memories/workspace/clash.md", + "owner version", + "agent" + ); + 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", + ]); + const ownerRoot = path.join(fixture.config.sessionsDir, "ws-owner", "memory"); + 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); + // Freed capacity lets a later pass of the SAME process fold in the rest: + // an incomplete pass is not memoized, since its retry depends on owner + // state the legacy check key does not observe. + 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(relisted).toHaveLength(MEMORY_MAX_FILES_PER_SCOPE); + expect(relisted.filter((f) => ["a.md", "b.md", "c.md", "d.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); + // Malformed (not missing) aborts too: a bad record whose source is + // gone could not be reconciled, and an empty substitute would drop the + // provenance for its owner copy while removal deletes the child. + for (const [label, body] of [ + ["not JSON", "{nope"], + ["not an object", "[]"], + ["record 'note.md'", JSON.stringify({ "note.md": { content: 1 } })], + ] as const) { + await fsPromises.writeFile(manifestPath, body); + expect( + await fixture.service + .adoptLegacyPrivateStoreForRemoval("ws-child", "ws-owner") + .then(() => null, getErrorMessage) + ).toContain(`malformed (${label})`); + expect(await pathExists(path.join(ownerRoot, "late.md"))).toBe(false); + } + await fsPromises.writeFile(manifestPath, savedManifest); + await fixture.service.adoptLegacyPrivateStoreForRemoval("ws-child", "ws-owner"); + expect(await pathExists(path.join(ownerRoot, "late.md"))).toBe(true); + }); + + 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 bd6ee6d380f..990c4e93713 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,12 @@ import { withTargetMutationLock, } from "@/node/services/refinement/targetMutationLocks"; import { memoryLogicalKey, type MemoryMetaService } from "@/node/services/memoryMeta"; +import { + adoptionTargetStamp, + legacyAdoptionManifestPath, + readLegacyAdoptionManifest, + type LegacyAdoptionRecord, +} from "@/node/services/memoryLegacyAdoption"; import { resolveWorkspaceMemoryOwnerId, workspaceMemoryOwnerResolver, @@ -378,8 +385,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 @@ -400,6 +422,88 @@ 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. 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 +546,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 +578,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,11 +592,12 @@ 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; } } @@ -752,6 +865,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 +907,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 +919,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 +1085,615 @@ 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); + 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, + } + ); + } + } + + /** + * 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; + let skipped = 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 readLegacyAdoptionManifest(manifestPath, { strict: true }); + 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 read gates as a memory command: containment (no symlink + // escape), size cap, and text-only (a lossy utf-8 decode cannot be + // carried by a text write). + const content = await legacy + .assertContained(relPath) + .then(() => this.readBoundedTextFile(legacy, relPath, relPath)) + .catch(() => null); + if (content === null || content.includes("\uFFFD")) { + // Dot-entries too (r73): `.note` is addressable, so a real note + // there may hold text `create` permitted (U+FFFD included) 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. + 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++; + 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) { + target = await this.legacyImportTarget(store, childId, relPath, content); + 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++; + continue; + } + const stagedStamp = await adoptionTargetStamp(stagingPath); + if (stagedStamp === null) { + await fsPromises.rm(stagingPath, { force: true }); + skipped++; + 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++; + 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++; + 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++; + 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. + // Only a complete pass is memoized: a note left unrepresented (owner + // store full, both destinations taken, unreadable, sidecar fold failed) + // is retried on the next access, and that retry depends on OWNER-side + // state the check key does not observe. + if (skipped === 0) { + this.legacyStoreCheckedAgainst.set(childId, checkKey); + } else { + this.legacyStoreCheckedAgainst.delete(childId); + } + if (adoptedCount > 0) this.emitChange(ctx, "workspace", "", "agent"); + return { skipped }; + } + + /** + * 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; + // A symlink is never a destination: the store's listing excludes links + // (and dotfiles), so a note "represented" through one would be + // invisible to the shared notebook, and a write would land through it. + const linkKind = await lstatKind(store.physicalPath(candidate)); + if (linkKind === "unreadable") { + throw new Error(`cannot inspect adoption destination ${candidate}`); + } + if (linkKind === "symlink") continue; + // Strict: a destination that merely could not be stat'ed (EACCES, EIO) + // is not free — declaring it so would overwrite whatever the owner + // keeps there once the copy runs. The failure aborts the pass instead + // (access-time: retried; removal: session intact). + const kind = await store.kind(candidate, { strict: true }); + if (kind === null) return { relPath: candidate, write: true }; + if (kind === "file") { + const existing = await this.readBoundedTextFile(store, candidate, candidate).catch( + () => null + ); + if (existing === 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 linkKind = await lstatKind(store.physicalPath(relPath)); + if (linkKind === "unreadable") throw new Error(`cannot inspect adopted copy ${relPath}`); + if (linkKind !== "other") return null; // missing, dir, or a symlink + try { + return await this.readBoundedTextFile(store, relPath, relPath); + } catch (error) { + if (error instanceof MemoryCommandError) return null; // over the cap + throw error; + } } /** From 1c5fc2021a9b79bf757f7bdb92ad966da63bbafc Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 13:44:02 +0000 Subject: [PATCH 3/9] =?UTF-8?q?=F0=9F=A4=96=20feat:=20seal=20a=20sub-agent?= =?UTF-8?q?'s=20shared-memory=20handover=20under=20the=20removal=20locks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removal of a sub-agent runs the legacy-notebook handover before any destructive step and again inside the removal lock set immediately before the tombstone (beforeTombstone), sealing the tombstone BEFORE the checkout is deleted so nothing can land in between; a handover that cannot place a note aborts the removal (SharedMemoryRemovalAbortedError) with the workspace intact, unless force accepts the loss. A checkout deletion refused after sealing rolls the tombstone back. The tombstone lease is held from before the earliest publish point. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$34.37`_ --- src/node/services/di/layers/core.ts | 1 + src/node/services/workspaceRemoval.test.ts | 137 ++++++++++++++ src/node/services/workspaceRemoval.ts | 181 ++++++++++++++---- src/node/services/workspaceService.test.ts | 207 ++++++++++++++++++++- src/node/services/workspaceService.ts | 167 ++++++++++++++--- 5 files changed, 626 insertions(+), 67 deletions(-) diff --git a/src/node/services/di/layers/core.ts b/src/node/services/di/layers/core.ts index 225ff77f860..4ed95035a5b 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/workspaceRemoval.test.ts b/src/node/services/workspaceRemoval.test.ts index 75abc00ae01..7b1c7a8a63e 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 1b5b06a1e52..da116567b8a 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 67270f9fb35..6fee92b654c 100644 --- a/src/node/services/workspaceService.test.ts +++ b/src/node/services/workspaceService.test.ts @@ -59,6 +59,7 @@ import type { } from "@/common/types/workspace"; import { makeAgentTaskIntegrationFake } from "./taskWorkspaceSeam.testUtils"; import { resolveWorkspaceMemoryOwnerId } from "./memoryWorkspaceOwner"; +import { isWorkspaceRemovalTombstoned } from "./workspaceRemoval"; import type { BackgroundProcessManager } from "./backgroundProcessManager"; import type { TerminalService } from "@/node/services/terminalService"; import type { DesktopSessionManager } from "@/node/services/desktop/DesktopSessionManager"; @@ -14676,6 +14677,169 @@ 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 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 +15323,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); diff --git a/src/node/services/workspaceService.ts b/src/node/services/workspaceService.ts index fa478f984c4..9ba2d9b15e0 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) => @@ -5819,6 +5860,14 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { // workspace registered and intact, so the finally lifts the consolidation // teardown gate the drains below installed. let removalCommitted = 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 +5905,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 +5953,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 +6030,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 +6058,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 +6078,26 @@ 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; } if (isMultiProject(metadata)) { @@ -6352,11 +6446,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,15 +6482,26 @@ 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 @@ -6412,26 +6512,19 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { // 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. + 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. removalCommitted = false; throw error; } 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 { @@ -6544,6 +6637,26 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { return Err(`Failed to remove workspace: ${message}`); } finally { if (!removalCommitted) { + // A sealed sub-agent whose checkout deletion was then refused + // (force=false) keeps its session dir and config entry: lift the + // tombstone again (ownership-checked, r66) so it stays usable. + if (sealedForRemoval) { + 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) { From 365d8d66793ba6153748c928ce121874d6c031df Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 13:46:07 +0000 Subject: [PATCH 4/9] =?UTF-8?q?=F0=9F=A4=96=20fix:=20settle=20the=20strict?= =?UTF-8?q?=20sidecar=20read=20rejection=20without=20awaiting=20a=20non-th?= =?UTF-8?q?enable?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$34.37`_ --- src/node/services/memoryMeta.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/node/services/memoryMeta.test.ts b/src/node/services/memoryMeta.test.ts index db225a7b9ff..67929e71baa 100644 --- a/src/node/services/memoryMeta.test.ts +++ b/src/node/services/memoryMeta.test.ts @@ -5,6 +5,7 @@ import * as fsPromises from "node:fs/promises"; import * as path from "node:path"; 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", () => { @@ -299,7 +300,11 @@ describe("MemoryMetaService", () => { const strictReader = spyOn(fsPromises, "readFile").mockImplementationOnce((() => Promise.reject(Object.assign(new Error("EACCES"), { code: "EACCES" }))) as never); try { - await expect(strict.getEntriesOrThrow()).rejects.toThrow(/could not be read/); + const failure = await strict.getEntriesOrThrow().then( + () => null, + (error: unknown) => error + ); + expect(getErrorMessage(failure)).toContain("could not be read"); } finally { strictReader.mockRestore(); } From 894bde611908746e9461c521ae42bb687fb5c5b5 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 14:08:24 +0000 Subject: [PATCH 5/9] =?UTF-8?q?=F0=9F=A4=96=20fix:=20adopt=20inactive=20de?= =?UTF-8?q?scendants=20on=20owner=20access,=20bound=20re-walks,=20quaranti?= =?UTF-8?q?ne=20malformed=20manifests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 1 on the legacy-notebook layer: - The owner's own workspace-memory access now runs the access-time adoption for every registered descendant resolving to it (one config snapshot per pass, one lstat per child without a legacy root), so a sub-agent that finished before the upgrade no longer has to touch memory itself for its notes to become visible. - The access-time pass memoizes its legacy-store check key even when notes were left unrepresented (owner store full, unrepresentable entries): an unchanged legacy store cannot adopt more on retry, so it is no longer re-walked on every access. Removal's strict pass still throws. - A malformed adoption manifest is quarantined beside itself (.malformed-) and the pass continues from an empty record map (re-adoption is idempotent); a failing quarantine rename or an unreadable manifest still fails closed. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$41.17`_ --- src/node/services/memoryLegacyAdoption.ts | 15 ++- src/node/services/memoryService.test.ts | 129 +++++++++++++++++----- src/node/services/memoryService.ts | 101 +++++++++++++++-- 3 files changed, 206 insertions(+), 39 deletions(-) diff --git a/src/node/services/memoryLegacyAdoption.ts b/src/node/services/memoryLegacyAdoption.ts index d2d3353e6b5..e1342e646d4 100644 --- a/src/node/services/memoryLegacyAdoption.ts +++ b/src/node/services/memoryLegacyAdoption.ts @@ -83,6 +83,19 @@ function parseLegacyAdoptionRecord(value: unknown): LegacyAdoptionRecord | null }; } +/** + * 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) @@ -111,7 +124,7 @@ export async function readLegacyAdoptionManifest( } const malformed = (detail: string): Map => { if (options?.strict === true) { - throw new Error(`the legacy adoption manifest at ${manifestPath} is malformed (${detail})`); + throw new LegacyAdoptionManifestMalformedError(manifestPath, detail); } return new Map(); }; diff --git a/src/node/services/memoryService.test.ts b/src/node/services/memoryService.test.ts index d73f7366c63..0ee176e9a18 100644 --- a/src/node/services/memoryService.test.ts +++ b/src/node/services/memoryService.test.ts @@ -1520,20 +1520,14 @@ describe("MemoryService", () => { 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. + // 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" }; - await fixture.service.create( - ownerCtx, - "/memories/workspace/sub/same.md", - "identical", - "agent" - ); - await fixture.service.create( - ownerCtx, - "/memories/workspace/clash.md", - "owner version", - "agent" - ); + 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)); @@ -1544,7 +1538,6 @@ describe("MemoryService", () => { "only-child.md", "sub/same.md", ]); - const ownerRoot = path.join(fixture.config.sessionsDir, "ws-owner", "memory"); expect(await fsPromises.readFile(path.join(ownerRoot, "only-child.md"), "utf-8")).toBe( "child notes" ); @@ -1711,17 +1704,34 @@ describe("MemoryService", () => { "agent" ); expect(full.success).toBe(false); - // Freed capacity lets a later pass of the SAME process fold in the rest: - // an incomplete pass is not memoized, since its retry depends on owner - // state the legacy check key does not observe. + // 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(relisted).toHaveLength(MEMORY_MAX_FILES_PER_SCOPE); + 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", ]); @@ -1838,27 +1848,90 @@ describe("MemoryService", () => { await fsPromises.writeFile(manifestPath, savedManifest); } expect(await pathExists(path.join(ownerRoot, "late.md"))).toBe(false); - // Malformed (not missing) aborts too: a bad record whose source is - // gone could not be reconciled, and an empty substitute would drop the - // provenance for its owner copy while removal deletes the child. - for (const [label, body] of [ - ["not JSON", "{nope"], - ["not an object", "[]"], - ["record 'note.md'", JSON.stringify({ "note.md": { content: 1 } })], - ] as const) { + // 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 (${label})`); - expect(await pathExists(path.join(ownerRoot, "late.md"))).toBe(false); + ).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("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); diff --git a/src/node/services/memoryService.ts b/src/node/services/memoryService.ts index 990c4e93713..492bf16b7f3 100644 --- a/src/node/services/memoryService.ts +++ b/src/node/services/memoryService.ts @@ -51,6 +51,7 @@ import { memoryLogicalKey, type MemoryMetaService } from "@/node/services/memory import { adoptionTargetStamp, legacyAdoptionManifestPath, + LegacyAdoptionManifestMalformedError, readLegacyAdoptionManifest, type LegacyAdoptionRecord, } from "@/node/services/memoryLegacyAdoption"; @@ -1142,6 +1143,11 @@ export class MemoryService extends EventEmitter { // 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 { @@ -1158,6 +1164,45 @@ export class MemoryService extends EventEmitter { } } + /** + * 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 @@ -1325,7 +1370,7 @@ export class MemoryService extends EventEmitter { // 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 readLegacyAdoptionManifest(manifestPath, { strict: true }); + 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 @@ -1617,19 +1662,55 @@ export class MemoryService extends EventEmitter { } // Recorded against the state observed BEFORE the pass: a foreign write // landing during it changes the stamp and re-runs the (idempotent) pass. - // Only a complete pass is memoized: a note left unrepresented (owner - // store full, both destinations taken, unreadable, sidecar fold failed) - // is retried on the next access, and that retry depends on OWNER-side - // state the check key does not observe. - if (skipped === 0) { - this.legacyStoreCheckedAgainst.set(childId, checkKey); - } else { - this.legacyStoreCheckedAgainst.delete(childId); - } + // Memoized even when notes were left unrepresented (owner store full, + // both destinations taken, unreadable, sidecar fold failed): 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. + this.legacyStoreCheckedAgainst.set(childId, checkKey); 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 From 2e15bad86d1d23678cbc4214d1fc5462a96d890e Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 14:33:52 +0000 Subject: [PATCH 6/9] =?UTF-8?q?=F0=9F=A4=96=20fix:=20retry=20transient=20a?= =?UTF-8?q?doption=20failures,=20decode=20legacy=20notes=20strictly,=20rol?= =?UTF-8?q?l=20back=20stale=20seals?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 2 on the legacy-notebook layer: - The adoption pass counts transient failures (a read, stage, install or sidecar write that failed) apart from permanent skips (over the cap, doubly conflicting, not text, escaping destination) and memoizes its check key only when none were transient, so a note behind a permission interval is retried on the next access while an unchanged over-cap notebook is not re-walked. Removal's strict pass is unchanged. - Legacy notes are read as bytes and decoded with a fatal UTF-8 decoder: a literal U+FFFD is adopted, invalid byte sequences are skipped. - The removal tombstone is rolled back whenever the removal ends with the workspace still registered (a teardown step failing after the seal, a refused checkout deletion, deregistration failing), not only before the runtime-deletion commit point; the consolidation teardown gate is released on the same truth and harvest records are finalized only after deregistration. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$51.16`_ --- src/node/services/memoryService.test.ts | 80 +++++++++++++++++++++ src/node/services/memoryService.ts | 79 ++++++++++++++------ src/node/services/workspaceService.test.ts | 83 ++++++++++++++++++++++ src/node/services/workspaceService.ts | 39 +++++----- 4 files changed, 245 insertions(+), 36 deletions(-) diff --git a/src/node/services/memoryService.test.ts b/src/node/services/memoryService.test.ts index 0ee176e9a18..5e44b0d2d8f 100644 --- a/src/node/services/memoryService.test.ts +++ b/src/node/services/memoryService.test.ts @@ -1888,6 +1888,86 @@ describe("MemoryService", () => { 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("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); diff --git a/src/node/services/memoryService.ts b/src/node/services/memoryService.ts index 492bf16b7f3..07134bd6663 100644 --- a/src/node/services/memoryService.ts +++ b/src/node/services/memoryService.ts @@ -410,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. */ @@ -604,11 +606,15 @@ class LocalMemoryStore implements MemoryStore { } 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(); } @@ -1343,7 +1349,13 @@ export class MemoryService extends EventEmitter { // 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); @@ -1394,18 +1406,32 @@ export class MemoryService extends EventEmitter { }); for (const relPath of files) { // Same read gates as a memory command: containment (no symlink - // escape), size cap, and text-only (a lossy utf-8 decode cannot be - // carried by a text write). - const content = await legacy - .assertContained(relPath) - .then(() => this.readBoundedTextFile(legacy, relPath, relPath)) - .catch(() => null); - if (content === null || content.includes("\uFFFD")) { - // Dot-entries too (r73): `.note` is addressable, so a real note - // there may hold text `create` permitted (U+FFFD included) 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. + // 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; } @@ -1454,6 +1480,7 @@ export class MemoryService extends EventEmitter { { childId, owner, relPath, target: previous.target, error } ); skipped++; + transientSkips++; continue; } if (priorContent === content) { @@ -1528,12 +1555,14 @@ export class MemoryService extends EventEmitter { error, }); skipped++; + transientSkips++; continue; } const stagedStamp = await adoptionTargetStamp(stagingPath); if (stagedStamp === null) { await fsPromises.rm(stagingPath, { force: true }); skipped++; + transientSkips++; continue; } adopted.set(relPath, { @@ -1567,6 +1596,7 @@ export class MemoryService extends EventEmitter { } ); skipped++; + transientSkips++; continue; } // The install: same session dir, so a plain rename (an EXDEV — the @@ -1585,6 +1615,7 @@ export class MemoryService extends EventEmitter { error, }); skipped++; + transientSkips++; continue; } remainingCapacity--; @@ -1633,6 +1664,7 @@ export class MemoryService extends EventEmitter { // 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; } } @@ -1662,13 +1694,20 @@ export class MemoryService extends EventEmitter { } // 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 unrepresented (owner store full, - // both destinations taken, unreadable, sidecar fold failed): 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. - this.legacyStoreCheckedAgainst.set(childId, checkKey); + // 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 }; } diff --git a/src/node/services/workspaceService.test.ts b/src/node/services/workspaceService.test.ts index 6fee92b654c..04bba3bd186 100644 --- a/src/node/services/workspaceService.test.ts +++ b/src/node/services/workspaceService.test.ts @@ -60,6 +60,8 @@ import type { 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"; @@ -14797,6 +14799,87 @@ describe("WorkspaceService remove sub-agent handover ordering", () => { } }); + 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(() => diff --git a/src/node/services/workspaceService.ts b/src/node/services/workspaceService.ts index 9ba2d9b15e0..fa844ea683a 100644 --- a/src/node/services/workspaceService.ts +++ b/src/node/services/workspaceService.ts @@ -5855,11 +5855,14 @@ 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. @@ -6098,6 +6101,7 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { this.lockedSharedMemoryHandover(workspaceId, sharedMemoryOwnerId, force), }); sealedForRemoval = true; + tombstonePublished = true; } if (isMultiProject(metadata)) { @@ -6395,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 @@ -6503,10 +6506,7 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { ? undefined : () => 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 @@ -6520,9 +6520,11 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { // 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. - removalCommitted = false; 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); } // The on-disk devtools.jsonl died with the session directory above; also drop any @@ -6604,6 +6606,10 @@ 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. + await this.memoryConsolidationService?.finalizeHarvestsForRemoval(workspaceId); // Deregistration succeeded: drop the workspace's activity/status entry // so extensionMetadata.json stays bounded (stale entries were @@ -6636,11 +6642,12 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { const message = getErrorMessage(error); return Err(`Failed to remove workspace: ${message}`); } finally { - if (!removalCommitted) { - // A sealed sub-agent whose checkout deletion was then refused - // (force=false) keeps its session dir and config entry: lift the - // tombstone again (ownership-checked, r66) so it stays usable. - if (sealedForRemoval) { + 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, From 6102b00bf8af94326f9c3afebbfaeaa9ac0b9aa2 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 15:02:54 +0000 Subject: [PATCH 7/9] =?UTF-8?q?=F0=9F=A4=96=20fix:=20fingerprint=20the=20w?= =?UTF-8?q?hole=20legacy=20store,=20combine=20sibling=20pins,=20validate?= =?UTF-8?q?=20legacy=20names?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round 3 on the legacy-notebook layer: - The legacy store's change stamp walks the FULL directory (strict, uncapped, sizes and mtimes only), so an edit to a note sorted past the capped listing re-runs the adoption pass; adoption itself stays capped. - A descendant's first adoption onto an identical owner note keeps the owner's pin only when the owner wrote that note; when the note is another descendant's adopted copy (its manifest records it as created), the descendants' pins combine (mergeKeys pinned: "either"). - Finalizing harvest records after deregistration is best-effort: a failure is logged and the committed removal still succeeds and emits its event. - A legacy file whose name the memory path grammar rejects is never copied into the shared store (a permanent skip removal still reports). --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$52.95`_ --- src/node/services/memoryMeta.ts | 17 ++-- src/node/services/memoryService.test.ts | 97 ++++++++++++++++++++++ src/node/services/memoryService.ts | 86 ++++++++++++++++--- src/node/services/workspaceService.test.ts | 22 +++++ src/node/services/workspaceService.ts | 11 ++- 5 files changed, 215 insertions(+), 18 deletions(-) diff --git a/src/node/services/memoryMeta.ts b/src/node/services/memoryMeta.ts index 5e2c086301c..dce0ee42d6e 100644 --- a/src/node/services/memoryMeta.ts +++ b/src/node/services/memoryMeta.ts @@ -236,14 +236,16 @@ export class MemoryMetaService { * 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 + * first adoption must not override the owner's own choice), follows the * source (`pinned: "source"`, the child changed it since the last - * adoption — see MemoryService.adoptLegacyPrivateStore). Idempotent. + * adoption), or combines (`pinned: "either"`, the target is itself an + * adopted copy with no owner choice behind it — see + * MemoryService.adoptLegacyPrivateStore). Idempotent. */ mergeKeys: ( sourceLogicalKey: string, targetLogicalKey: string, - options: { pinned: "target" | "source" } + options: { pinned: "target" | "source" | "either" } ): Effect.Effect => this.mutate((entries) => { for (const [key, source] of Object.entries(entries)) { @@ -254,7 +256,12 @@ export class MemoryMetaService { target === undefined ? { ...source } : { - pinned: options.pinned === "source" ? source.pinned : target.pinned, + pinned: + options.pinned === "source" + ? source.pinned + : options.pinned === "either" + ? source.pinned || target.pinned + : target.pinned, accessCount: Math.max(target.accessCount, source.accessCount), lastAccessedAt: maxTimestamp(target.lastAccessedAt, source.lastAccessedAt), lastWriteAt: maxTimestamp(target.lastWriteAt, source.lastWriteAt), @@ -438,7 +445,7 @@ export class MemoryMetaService { async mergeKeys( sourceLogicalKey: string, targetLogicalKey: string, - options: { pinned: "target" | "source" } + options: { pinned: "target" | "source" | "either" } ): Promise { await Effect.runPromise(this.effects.mergeKeys(sourceLogicalKey, targetLogicalKey, options)); } diff --git a/src/node/services/memoryService.test.ts b/src/node/services/memoryService.test.ts index 5e44b0d2d8f..1d0e46cb964 100644 --- a/src/node/services/memoryService.test.ts +++ b/src/node/services/memoryService.test.ts @@ -1968,6 +1968,103 @@ describe("MemoryService", () => { await fixture.service.adoptLegacyPrivateStoreForRemoval("ws-child", "ws-owner"); }); + it("fingerprints the whole legacy store, so an edit past the cap prefix re-runs the pass", async () => { + using fixture = await createFixture("ws-child"); + await registerTaskTree(fixture); + const legacyRoot = path.join(fixture.config.sessionsDir, "ws-child", "memory"); + const ownerRoot = path.join(fixture.config.sessionsDir, "ws-owner", "memory"); + await fsPromises.mkdir(legacyRoot, { recursive: true }); + await fsPromises.mkdir(ownerRoot, { recursive: true }); + // The shared store is already full, and the legacy store holds more + // notes than the capped listing shows: every note is permanently + // skipped (no capacity), so the pass is memoized. + await Promise.all([ + ...Array.from({ length: MEMORY_MAX_FILES_PER_SCOPE }, (_, i) => + fsPromises.writeFile(path.join(ownerRoot, `o${String(i).padStart(4, "0")}.md`), "o") + ), + ...Array.from({ length: MEMORY_MAX_FILES_PER_SCOPE }, (_, i) => + fsPromises.writeFile(path.join(legacyRoot, `n${String(i).padStart(4, "0")}.md`), "n") + ), + ]); + await fsPromises.writeFile(path.join(legacyRoot, "zz-a.md"), "a"); + await fsPromises.writeFile(path.join(legacyRoot, "zz-b.md"), "b"); + const passes = spyOn( + fixture.service as unknown as { readOrQuarantineAdoptionManifest: () => Promise }, + "readOrQuarantineAdoptionManifest" + ); + await fixture.service.listIndexEntries({ ...fixture.ctx }); + expect(passes).toHaveBeenCalledTimes(1); + await fixture.service.listIndexEntries({ ...fixture.ctx }); + expect(passes).toHaveBeenCalledTimes(1); + // A downgraded build edits the note sorted beyond the capped listing: + // the fingerprint must see it (size change), not just the cap prefix. + await fsPromises.writeFile(path.join(legacyRoot, "zz-b.md"), "b edited on the old build"); + await fixture.service.listIndexEntries({ ...fixture.ctx }); + expect(passes).toHaveBeenCalledTimes(2); + }, 30_000); + + it("combines two descendants' pins on one adopted copy instead of keeping the first one's", async () => { + using fixture = await createFixture("ws-owner"); + await registerTaskTree(fixture); + // Same note, same bytes, in two pre-sharing descendants; only the one + // adopted second had it pinned. + for (const child of ["ws-child", "ws-grandchild"]) { + const legacyRoot = path.join(fixture.config.sessionsDir, child, "memory"); + await fsPromises.mkdir(legacyRoot, { recursive: true }); + await fsPromises.writeFile(path.join(legacyRoot, "same.md"), "shared lesson"); + } + await fixture.metaService.setPinned("workspace:ws-grandchild:same.md", true); + await fixture.service.listIndexEntries(fixture.ctx); + expect( + await fsPromises.readFile( + path.join(fixture.config.sessionsDir, "ws-owner", "memory", "same.md"), + "utf-8" + ) + ).toBe("shared lesson"); + // The owner copy is the first descendant's adopted copy, not an owner + // choice: the second descendant's pin is not dropped against it. + expect(await fixture.metaService.getPinnedKeys()).toContain("workspace:ws-owner:same.md"); + + // An owner's OWN note (no adoption created it) keeps the owner's pin + // state on a descendant's first adoption, as before. + const ownerRoot = path.join(fixture.config.sessionsDir, "ws-owner", "memory"); + await fsPromises.writeFile(path.join(ownerRoot, "mine.md"), "owner wrote this"); + await fixture.metaService.recordAccess("workspace:ws-owner:mine.md", { write: true }); + const legacyRoot = path.join(fixture.config.sessionsDir, "ws-child", "memory"); + await fsPromises.writeFile(path.join(legacyRoot, "mine.md"), "owner wrote this"); + await fixture.metaService.setPinned("workspace:ws-child:mine.md", true); + await fixture.service.listIndexEntries({ ...fixture.ctx }); + expect(await fixture.metaService.getPinnedKeys()).not.toContain("workspace:ws-owner:mine.md"); + }); + + 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); diff --git a/src/node/services/memoryService.ts b/src/node/services/memoryService.ts index 07134bd6663..44c0b510df1 100644 --- a/src/node/services/memoryService.ts +++ b/src/node/services/memoryService.ts @@ -459,10 +459,14 @@ function legacySidecarPinned(sidecar: string): boolean | 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 + * mtime and every 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. Missing pieces read as fixed tokens. + * self-fallback write does. The walk is the FULL directory (strict, + * uncapped, no file reads): the adoption pass itself stops at the shared + * store's capacity, but a note sorted past the cap prefix still belongs to + * the store's state — fingerprinting only the capped listing would let an + * edit there leave the memo valid forever. Paid only while a legacy + * directory exists. Missing or unlistable pieces read as fixed tokens. */ async function legacyStoreStamp(legacyRoot: string): Promise { const rootMtime = await fsPromises @@ -470,8 +474,8 @@ async function legacyStoreStamp(legacyRoot: string): Promise { .then((stat) => String(stat.mtimeMs)) .catch(() => "missing"); const files = await new LocalMemoryStore(legacyRoot) - .listFiles({ includeDotfiles: true }) - .catch(() => []); + .listFiles({ strict: true, includeDotfiles: true }) + .catch(() => ["\u0002unlistable"]); const fileStamps = await Promise.all( files.map(async (relPath) => { const stamp = await fsPromises @@ -1405,12 +1409,23 @@ export class MemoryService extends EventEmitter { encoding: "utf-8", }); for (const relPath of files) { - // Same read gates as a memory command: 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. + // 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); @@ -1646,6 +1661,15 @@ export class MemoryService extends EventEmitter { // 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; + // A first adoption onto an identical owner note keeps the owner's + // pin — unless that note is itself another descendant's adopted + // copy, in which case no owner choice exists and the descendants' + // pins combine (an unpinned sibling adopted first must not drop + // this child's pin). + const siblingCopy = + previous === undefined && + !target.write && + (await this.targetCreatedByDescendantAdoption(owner, childId, target.relPath)); try { await this.metaService.mergeKeys( childKey, @@ -1653,7 +1677,10 @@ export class MemoryService extends EventEmitter { projectPath: ctx.projectPath, workspaceId: owner, }), - { pinned: childPinChanged && foldChildPin ? "source" : "target" } + { + pinned: + childPinChanged && foldChildPin ? "source" : siblingCopy ? "either" : "target", + } ); } catch (error) { log.warn( @@ -1750,6 +1777,41 @@ export class MemoryService extends EventEmitter { } } + /** + * Whether `targetRelPath` in the owner store is a copy some OTHER + * descendant's adoption created (its manifest records the target as + * `created`), as opposed to a note the owner wrote itself. Tolerant reads: + * an unreadable sibling manifest answers "no", which keeps the owner's pin + * — today's behavior. + */ + private async targetCreatedByDescendantAdoption( + owner: string, + childId: string, + targetRelPath: string + ): Promise { + const cfg = this.config.loadConfigOrDefault(); + const resolve = workspaceMemoryOwnerResolver(cfg); + for (const project of cfg.projects.values()) { + for (const sibling of project.workspaces) { + const id = sibling.id; + if (id === undefined || id === childId || id === owner || resolve(id) !== owner) continue; + const manifest = await readLegacyAdoptionManifest( + legacyAdoptionManifestPath(path.join(this.config.sessionsDir, id)) + ); + for (const record of manifest.values()) { + if ( + record.target === targetRelPath && + record.created === true && + record.pending !== true + ) { + return true; + } + } + } + } + return false; + } + /** * 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 diff --git a/src/node/services/workspaceService.test.ts b/src/node/services/workspaceService.test.ts index 04bba3bd186..4c9fb9f3839 100644 --- a/src/node/services/workspaceService.test.ts +++ b/src/node/services/workspaceService.test.ts @@ -15459,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 fa844ea683a..c4dfb5fa6aa 100644 --- a/src/node/services/workspaceService.ts +++ b/src/node/services/workspaceService.ts @@ -6609,7 +6609,16 @@ export class WorkspaceService extends EventEmitter implements WorkspaceHost { // 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. - await this.memoryConsolidationService?.finalizeHarvestsForRemoval(workspaceId); + // 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 From 08d1a7a09cc66527c253d705e4db29bcef1d715d Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 15:29:35 +0000 Subject: [PATCH 8/9] =?UTF-8?q?=F0=9F=A4=96=20fix:=20revert=20sibling-pin?= =?UTF-8?q?=20combining=20and=20full-store=20fingerprint=20(moved=20to=20l?= =?UTF-8?q?ater=20layers)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scope reduction after review: keep this layer at owner-side adoption plus the R1/R2 hardening. The two R3 additions below grew scope beyond the layer and are deferred: - legacyStoreStamp goes back to the capped, non-strict listing; an over-cap legacy store fingerprints only the capped prefix (edits past it are picked up by a restart or removal's forced pass). The throttled full-store fingerprint lands with the multi-backend layer. - mergeKeys drops the `pinned: "either"` mode and MemoryService no longer inspects sibling manifests to combine descendants' pins on a first adoption; the owner's pin stands as before. Kept from R3: best-effort finalize in workspaceService and the unaddressable-legacy-name skip with its test. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$77.94`_ --- src/node/services/memoryMeta.ts | 17 ++---- src/node/services/memoryService.test.ts | 69 ------------------------- src/node/services/memoryService.ts | 67 ++++-------------------- 3 files changed, 15 insertions(+), 138 deletions(-) diff --git a/src/node/services/memoryMeta.ts b/src/node/services/memoryMeta.ts index dce0ee42d6e..5e2c086301c 100644 --- a/src/node/services/memoryMeta.ts +++ b/src/node/services/memoryMeta.ts @@ -236,16 +236,14 @@ export class MemoryMetaService { * 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), follows the + * first adoption must not override the owner's own choice) or follows the * source (`pinned: "source"`, the child changed it since the last - * adoption), or combines (`pinned: "either"`, the target is itself an - * adopted copy with no owner choice behind it — see - * MemoryService.adoptLegacyPrivateStore). Idempotent. + * adoption — see MemoryService.adoptLegacyPrivateStore). Idempotent. */ mergeKeys: ( sourceLogicalKey: string, targetLogicalKey: string, - options: { pinned: "target" | "source" | "either" } + options: { pinned: "target" | "source" } ): Effect.Effect => this.mutate((entries) => { for (const [key, source] of Object.entries(entries)) { @@ -256,12 +254,7 @@ export class MemoryMetaService { target === undefined ? { ...source } : { - pinned: - options.pinned === "source" - ? source.pinned - : options.pinned === "either" - ? source.pinned || target.pinned - : target.pinned, + 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), @@ -445,7 +438,7 @@ export class MemoryMetaService { async mergeKeys( sourceLogicalKey: string, targetLogicalKey: string, - options: { pinned: "target" | "source" | "either" } + options: { pinned: "target" | "source" } ): Promise { await Effect.runPromise(this.effects.mergeKeys(sourceLogicalKey, targetLogicalKey, options)); } diff --git a/src/node/services/memoryService.test.ts b/src/node/services/memoryService.test.ts index 1d0e46cb964..6084e53e194 100644 --- a/src/node/services/memoryService.test.ts +++ b/src/node/services/memoryService.test.ts @@ -1968,75 +1968,6 @@ describe("MemoryService", () => { await fixture.service.adoptLegacyPrivateStoreForRemoval("ws-child", "ws-owner"); }); - it("fingerprints the whole legacy store, so an edit past the cap prefix re-runs the pass", async () => { - using fixture = await createFixture("ws-child"); - await registerTaskTree(fixture); - const legacyRoot = path.join(fixture.config.sessionsDir, "ws-child", "memory"); - const ownerRoot = path.join(fixture.config.sessionsDir, "ws-owner", "memory"); - await fsPromises.mkdir(legacyRoot, { recursive: true }); - await fsPromises.mkdir(ownerRoot, { recursive: true }); - // The shared store is already full, and the legacy store holds more - // notes than the capped listing shows: every note is permanently - // skipped (no capacity), so the pass is memoized. - await Promise.all([ - ...Array.from({ length: MEMORY_MAX_FILES_PER_SCOPE }, (_, i) => - fsPromises.writeFile(path.join(ownerRoot, `o${String(i).padStart(4, "0")}.md`), "o") - ), - ...Array.from({ length: MEMORY_MAX_FILES_PER_SCOPE }, (_, i) => - fsPromises.writeFile(path.join(legacyRoot, `n${String(i).padStart(4, "0")}.md`), "n") - ), - ]); - await fsPromises.writeFile(path.join(legacyRoot, "zz-a.md"), "a"); - await fsPromises.writeFile(path.join(legacyRoot, "zz-b.md"), "b"); - const passes = spyOn( - fixture.service as unknown as { readOrQuarantineAdoptionManifest: () => Promise }, - "readOrQuarantineAdoptionManifest" - ); - await fixture.service.listIndexEntries({ ...fixture.ctx }); - expect(passes).toHaveBeenCalledTimes(1); - await fixture.service.listIndexEntries({ ...fixture.ctx }); - expect(passes).toHaveBeenCalledTimes(1); - // A downgraded build edits the note sorted beyond the capped listing: - // the fingerprint must see it (size change), not just the cap prefix. - await fsPromises.writeFile(path.join(legacyRoot, "zz-b.md"), "b edited on the old build"); - await fixture.service.listIndexEntries({ ...fixture.ctx }); - expect(passes).toHaveBeenCalledTimes(2); - }, 30_000); - - it("combines two descendants' pins on one adopted copy instead of keeping the first one's", async () => { - using fixture = await createFixture("ws-owner"); - await registerTaskTree(fixture); - // Same note, same bytes, in two pre-sharing descendants; only the one - // adopted second had it pinned. - for (const child of ["ws-child", "ws-grandchild"]) { - const legacyRoot = path.join(fixture.config.sessionsDir, child, "memory"); - await fsPromises.mkdir(legacyRoot, { recursive: true }); - await fsPromises.writeFile(path.join(legacyRoot, "same.md"), "shared lesson"); - } - await fixture.metaService.setPinned("workspace:ws-grandchild:same.md", true); - await fixture.service.listIndexEntries(fixture.ctx); - expect( - await fsPromises.readFile( - path.join(fixture.config.sessionsDir, "ws-owner", "memory", "same.md"), - "utf-8" - ) - ).toBe("shared lesson"); - // The owner copy is the first descendant's adopted copy, not an owner - // choice: the second descendant's pin is not dropped against it. - expect(await fixture.metaService.getPinnedKeys()).toContain("workspace:ws-owner:same.md"); - - // An owner's OWN note (no adoption created it) keeps the owner's pin - // state on a descendant's first adoption, as before. - const ownerRoot = path.join(fixture.config.sessionsDir, "ws-owner", "memory"); - await fsPromises.writeFile(path.join(ownerRoot, "mine.md"), "owner wrote this"); - await fixture.metaService.recordAccess("workspace:ws-owner:mine.md", { write: true }); - const legacyRoot = path.join(fixture.config.sessionsDir, "ws-child", "memory"); - await fsPromises.writeFile(path.join(legacyRoot, "mine.md"), "owner wrote this"); - await fixture.metaService.setPinned("workspace:ws-child:mine.md", true); - await fixture.service.listIndexEntries({ ...fixture.ctx }); - expect(await fixture.metaService.getPinnedKeys()).not.toContain("workspace:ws-owner:mine.md"); - }); - it("never copies a legacy note whose name the memory path grammar rejects", async () => { using fixture = await createFixture("ws-child"); await registerTaskTree(fixture); diff --git a/src/node/services/memoryService.ts b/src/node/services/memoryService.ts index 44c0b510df1..9c14e7f3095 100644 --- a/src/node/services/memoryService.ts +++ b/src/node/services/memoryService.ts @@ -459,14 +459,14 @@ function legacySidecarPinned(sidecar: string): boolean | null { /** * Change stamp of a sub-agent's legacy private store: the root directory's - * mtime and every file's size + mtime — a DOWNGRADED build editing an + * 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. The walk is the FULL directory (strict, - * uncapped, no file reads): the adoption pass itself stops at the shared - * store's capacity, but a note sorted past the cap prefix still belongs to - * the store's state — fingerprinting only the capped listing would let an - * edit there leave the memo valid forever. Paid only while a legacy - * directory exists. Missing or unlistable pieces read as fixed tokens. + * 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 @@ -474,8 +474,8 @@ async function legacyStoreStamp(legacyRoot: string): Promise { .then((stat) => String(stat.mtimeMs)) .catch(() => "missing"); const files = await new LocalMemoryStore(legacyRoot) - .listFiles({ strict: true, includeDotfiles: true }) - .catch(() => ["\u0002unlistable"]); + .listFiles({ includeDotfiles: true }) + .catch(() => []); const fileStamps = await Promise.all( files.map(async (relPath) => { const stamp = await fsPromises @@ -1661,15 +1661,6 @@ export class MemoryService extends EventEmitter { // 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; - // A first adoption onto an identical owner note keeps the owner's - // pin — unless that note is itself another descendant's adopted - // copy, in which case no owner choice exists and the descendants' - // pins combine (an unpinned sibling adopted first must not drop - // this child's pin). - const siblingCopy = - previous === undefined && - !target.write && - (await this.targetCreatedByDescendantAdoption(owner, childId, target.relPath)); try { await this.metaService.mergeKeys( childKey, @@ -1677,10 +1668,7 @@ export class MemoryService extends EventEmitter { projectPath: ctx.projectPath, workspaceId: owner, }), - { - pinned: - childPinChanged && foldChildPin ? "source" : siblingCopy ? "either" : "target", - } + { pinned: childPinChanged && foldChildPin ? "source" : "target" } ); } catch (error) { log.warn( @@ -1777,41 +1765,6 @@ export class MemoryService extends EventEmitter { } } - /** - * Whether `targetRelPath` in the owner store is a copy some OTHER - * descendant's adoption created (its manifest records the target as - * `created`), as opposed to a note the owner wrote itself. Tolerant reads: - * an unreadable sibling manifest answers "no", which keeps the owner's pin - * — today's behavior. - */ - private async targetCreatedByDescendantAdoption( - owner: string, - childId: string, - targetRelPath: string - ): Promise { - const cfg = this.config.loadConfigOrDefault(); - const resolve = workspaceMemoryOwnerResolver(cfg); - for (const project of cfg.projects.values()) { - for (const sibling of project.workspaces) { - const id = sibling.id; - if (id === undefined || id === childId || id === owner || resolve(id) !== owner) continue; - const manifest = await readLegacyAdoptionManifest( - legacyAdoptionManifestPath(path.join(this.config.sessionsDir, id)) - ); - for (const record of manifest.values()) { - if ( - record.target === targetRelPath && - record.created === true && - record.pending !== true - ) { - return true; - } - } - } - } - return false; - } - /** * 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 From 4fdba459a43f1e67679d69065df41f0671645a45 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 11 Sep 2026 16:27:36 +0000 Subject: [PATCH 9/9] =?UTF-8?q?=F0=9F=A4=96=20fix:=20inspect=20adoption=20?= =?UTF-8?q?destinations=20by=20type=20before=20reading,=20compare=20bytes?= =?UTF-8?q?=20strictly,=20retry=20unreadable=20ones?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The destination-side comparison of the legacy adoption pass (both the placement in legacyImportTarget and the prior-copy re-check in inspectAdoptedCopy) now goes through one helper, inspectAdoptionDestination: - lstat first, regular file required: a FIFO/socket/device, directory or symlink at a destination is occupied owner state and is never opened (open() on a FIFO blocks until a peer appears and hung the pass). - destination bytes are read raw and decoded strictly, so an owner note holding invalid UTF-8 no longer settles a legacy note that literally contains U+FFFD as "already present"; the legacy note is copied byte-exact under imported// instead. - an EACCES/EIO on the destination's lstat or read propagates as a TRANSIENT skip (no copy, no manifest record, pass not memoized, retried on the next access) instead of being read as a mismatch that duplicated the note under imported//; removal still refuses on skipped > 0. Tests: FIFO at both slots (skips if mkfifo is unavailable), U+FFFD vs invalid-UTF-8 pair, EACCES on the destination then cleared. --- _Generated with `xum` • Model: `anthropic:claude-fable-5-1` • Thinking: `high` • Cost: `$81.16`_ --- src/node/services/memoryService.test.ts | 111 ++++++++++++++++++++++++ src/node/services/memoryService.ts | 80 +++++++++++------ 2 files changed, 165 insertions(+), 26 deletions(-) diff --git a/src/node/services/memoryService.test.ts b/src/node/services/memoryService.test.ts index 6084e53e194..2d56da8d057 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"; @@ -1968,6 +1969,116 @@ describe("MemoryService", () => { 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); diff --git a/src/node/services/memoryService.ts b/src/node/services/memoryService.ts index 9c14e7f3095..8ae65ce29d3 100644 --- a/src/node/services/memoryService.ts +++ b/src/node/services/memoryService.ts @@ -1522,7 +1522,22 @@ export class MemoryService extends EventEmitter { } } if (target === null) { - target = await this.legacyImportTarget(store, childId, relPath, content); + // 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; @@ -1784,26 +1799,9 @@ export class MemoryService extends EventEmitter { () => false ); if (!contained) continue; - // A symlink is never a destination: the store's listing excludes links - // (and dotfiles), so a note "represented" through one would be - // invisible to the shared notebook, and a write would land through it. - const linkKind = await lstatKind(store.physicalPath(candidate)); - if (linkKind === "unreadable") { - throw new Error(`cannot inspect adoption destination ${candidate}`); - } - if (linkKind === "symlink") continue; - // Strict: a destination that merely could not be stat'ed (EACCES, EIO) - // is not free — declaring it so would overwrite whatever the owner - // keeps there once the copy runs. The failure aborts the pass instead - // (access-time: retried; removal: session intact). - const kind = await store.kind(candidate, { strict: true }); - if (kind === null) return { relPath: candidate, write: true }; - if (kind === "file") { - const existing = await this.readBoundedTextFile(store, candidate, candidate).catch( - () => null - ); - if (existing === content) return { relPath: candidate, write: false }; - } + 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; } @@ -1820,15 +1818,45 @@ export class MemoryService extends EventEmitter { () => false ); if (!contained) return null; - const linkKind = await lstatKind(store.physicalPath(relPath)); - if (linkKind === "unreadable") throw new Error(`cannot inspect adopted copy ${relPath}`); - if (linkKind !== "other") return null; // missing, dir, or a symlink + 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 { - return await this.readBoundedTextFile(store, relPath, relPath); + isRegularFile = (await fsPromises.lstat(store.physicalPath(relPath))).isFile(); } catch (error) { - if (error instanceof MemoryCommandError) return null; // over the cap + 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 }; + } } /**