From 84205bcb320504196dea527cc6b6ccf4c918724a Mon Sep 17 00:00:00 2001 From: Sarav Date: Tue, 29 Sep 2026 08:31:24 +0530 Subject: [PATCH 1/5] fix(memory): a record deleted out from under us is re-created, not updated forever MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The mirror index names the cloud record for a block. When the record set came back WITHOUT that record — deleted in the web app, by another client, or by a wipe — absence read as "still ours": const indexUsable = existing?.memoryId && (!indexed || !isArchived(indexed)) `!indexed` is truthy exactly when the record is gone, so `match` stayed pointing at a dead id and the sweep issued `MemoryApi.update(deadId, ...)`. `runQueue` counts that as one failed task and moves on without clearing the entry, so every later sweep retried the same dead id: the block never got back to the cloud, and the failure repeated for as long as the index entry survived. Absence now falls through to the identity search, which finds nothing and creates a fresh record — and the create rewrites the stale entry, so the state self-heals rather than needing a manual index wipe. Only a COMPLETE read counts as evidence of absence. A truncated set may simply not reach the record, and treating that as gone would create a second record for a block that already has one — the duplicate this path works hardest to avoid. The existing truncation guard then blocks the create anyway, but it would have done so after the index had already been discarded, which is the wrong order to decide it in. Unchanged: an archived record is still a tombstone and still sends the block to the identity search, because it IS present in the set. Two tests, each verified by mutation: dropping the absence check fails the re-create test, and dropping `!view.truncated` fails the unreachable-record test. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Tneq4MLNoRLsX7htV6fGZC --- .../src/altimate/workspace/memory-sync.ts | 14 ++++++- .../altimate/workspace/memory-sync.test.ts | 41 +++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/packages/opencode/src/altimate/workspace/memory-sync.ts b/packages/opencode/src/altimate/workspace/memory-sync.ts index 0e9e8c7c0..a5e2fa671 100644 --- a/packages/opencode/src/altimate/workspace/memory-sync.ts +++ b/packages/opencode/src/altimate/workspace/memory-sync.ts @@ -499,7 +499,19 @@ async function push( const indexed = existing?.memoryId ? view.records.find((r) => r.id === existing.memoryId) : undefined - const indexUsable = existing?.memoryId && (!indexed || !isArchived(indexed)) + // A record the index names but a COMPLETE record set does not hold is gone — + // deleted in the web app, by another client, or by a wipe. Absence used to + // read as "still ours", so `match` stayed pointing at a dead id and every + // sweep issued an update against a record that no longer existed: the block + // never got back to the cloud and the failure repeated for as long as the + // index entry survived. Falling through to the identity search instead lets + // the block be re-created, and the create rewrites the stale entry. + // + // Only a complete read counts. A truncated set may simply not reach the + // record, and treating that as gone would create a second record for a block + // that already has one — the duplicate this function works hardest to avoid. + const indexedGone = existing?.memoryId !== undefined && indexed === undefined && !view.truncated + const indexUsable = existing?.memoryId && !indexedGone && (!indexed || !isArchived(indexed)) const match = (indexUsable ? existing?.memoryId : undefined) ?? view.records.find((r) => isSameBlock(r, block, binding))?.id if (match) { // Refuse to move a record backwards. Two machines editing the same block, diff --git a/packages/opencode/test/altimate/workspace/memory-sync.test.ts b/packages/opencode/test/altimate/workspace/memory-sync.test.ts index 008151e87..01d84a9d8 100644 --- a/packages/opencode/test/altimate/workspace/memory-sync.test.ts +++ b/packages/opencode/test/altimate/workspace/memory-sync.test.ts @@ -677,6 +677,47 @@ describe("mirrorBlock", () => { expect(callsTo("/datamates/memory/mem-tomb", "PATCH").length).toBe(0) }) + test("a record deleted out from under us is re-created, not updated forever", async () => { + // The index named a record the cloud no longer holds — deleted in the web + // app, by another client, or by a wipe. Absence read as "still ours", so + // the match stayed pointing at a dead id and every sweep PATCHed a record + // that did not exist: the block never got back to the cloud, and the + // failure repeated for as long as the index entry survived. + const b = block({ id: "deleted-in-the-web-app" }) + createResult = [{ id: "mem-gone" }] + await mirrorBlock(b) + + listResponse = listResponse.filter((r: any) => r.id !== "mem-gone") + createResult = [{ id: "mem-fresh" }] + captured = [] + await mirrorBlock({ ...b, content: "still here", updated: "2027-01-01T00:00:00.000Z" }) + + expect(callsTo("/datamates/memory/mem-gone", "PATCH").length).toBe(0) + expect(callsTo("/datamates/memory/", "POST").length).toBe(1) + }) + + test("a truncated read does not mistake an unreachable record for a deleted one", async () => { + // Absence only means "gone" when the read was complete. A cut-short set may + // simply not reach the record, and creating a second one for a block that + // already has one is the duplicate this path works hardest to avoid — so + // the index stays usable and the record is updated where it is. + const b = block({ id: "past-the-window-but-indexed" }) + createResult = [{ id: "mem-far" }] + await mirrorBlock(b) + + // Its own record is no longer in the window, and the window is full. + listResponse = Array.from({ length: LIST_LIMIT }, (_, i) => ({ + id: `mem-${i}`, + memory: "other", + metadata: { source: MIRROR_SOURCE, block_id: `other-${i}`, block_scope: "global" }, + })) + captured = [] + await mirrorBlock({ ...b, content: "changed", updated: "2027-01-01T00:00:00.000Z" }) + + expect(callsTo("/datamates/memory/mem-far", "PATCH").length).toBe(1) + expect(callsTo("/datamates/memory/", "POST").length).toBe(0) + }) + test("a recreated block gets a fresh record rather than un-archiving the old one", async () => { // `MemoryApi.update` replaces metadata wholesale, so updating the tombstone // would drop `archived` and bring the deleted record back to life. From a7e4d3907f1f8f9186778dbd0919787d36c53d7e Mon Sep 17 00:00:00 2001 From: Sarav Date: Tue, 29 Sep 2026 11:19:04 +0530 Subject: [PATCH 2/5] feat(memory): a record archived elsewhere removes its local block MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Removal was one-directional. Deleting a block locally archives its cloud record (`MemoryStore.remove` -> `archiveBlock`), but archiving from the other side left the block sitting on disk. Prompt injection reads the LOCAL block, so the note kept being used with nothing to stop it — not as a race, as the steady state — and the next edit re-created the record, because the identity search skips archived ones. "Removed" meant "hidden from one table" and nothing more. This is the other direction. On the read path, which already holds the whole record set, a settled binding and the directory they belong to, a record this client mirrored and archived now removes the block it mirrors. Hooked into `loadWorkspaceMemory` rather than the sweep so it lands on the next hydrate or refresh instead of waiting for a bind, and awaited rather than detached: the blocks below it are what this load will serve, and reaping afterwards would leave one session still reading what was just deleted. Conservative by construction, and each guard is pinned by a test whose mutation was verified to fail it: - only a record carrying this client's `source`, and only an archived one - project blocks must match BOTH the workspace and the project key — two projects in one workspace may hold the same block id, and matching the workspace alone would delete the other project's file - never a block edited after `archived_at`: that is work written since the decision to remove, and the ordinary push re-mirrors it - a read failure leaves the block alone `MemoryStore.remove` is reused rather than unlinking, so a reaped block also gets the audit-log DELETE entry and is indistinguishable from one deleted here; the re-archive it triggers is a no-op, since the identity search excludes archived records. `MemoryStore.remove` takes an optional directory. Without it both the path and the binding resolve from the ambient instance, so reaping for a project other than the current one would have deleted the wrong file and archived the wrong workspace's record. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Tneq4MLNoRLsX7htV6fGZC --- .../src/altimate/workspace/memory-sync.ts | 100 +++++++++++++++ packages/opencode/src/memory/store.ts | 18 ++- .../altimate/workspace/memory-sync.test.ts | 116 ++++++++++++++++++ 3 files changed, 231 insertions(+), 3 deletions(-) diff --git a/packages/opencode/src/altimate/workspace/memory-sync.ts b/packages/opencode/src/altimate/workspace/memory-sync.ts index a5e2fa671..254fa0be5 100644 --- a/packages/opencode/src/altimate/workspace/memory-sync.ts +++ b/packages/opencode/src/altimate/workspace/memory-sync.ts @@ -118,6 +118,10 @@ export const syncInternals: { resolveBinding?: () => Promise /** Test seam for the local-existence check. Production reads the store. */ blockExists?: (block: MemoryBlock, directory?: string) => Promise + /** Test seams for the archived-record reaper. Production reads and writes the + * store; both are here so a test can observe a removal without a real file. */ + readBlock?: (scope: "global" | "project", id: string, directory?: string) => Promise + removeBlock?: (scope: "global" | "project", id: string, directory?: string) => Promise } = {} /** Instance.directory throws synchronously with no instance context, so a @@ -442,6 +446,92 @@ async function existsLocally(block: MemoryBlock, directory?: string): Promise `archiveBlock`), but archiving from the web + * app left the block sitting on disk — where prompt injection keeps reading it, + * and where the next edit re-creates the record because the identity search + * skips archived ones. So "deleted in the web app" meant "hidden from one web + * table" and nothing more. This is the other direction. + * + * Deliberately conservative. It acts only on a record this client mirrored and + * archived, only for a block that belongs to this binding, and never on a block + * edited after the archive was written — there the user's later edit wins and + * the ordinary push re-mirrors it. Everything else is left alone. + * + * Reuses `MemoryStore.remove` rather than unlinking: that path also writes the + * audit-log DELETE entry and re-archives the record, so a reaped block is + * indistinguishable from one the user deleted here. The re-archive is a no-op — + * the identity search excludes archived records, so it finds nothing to write. + * + * Imported lazily, like `existsLocally`: `@/memory/store` reaches this module on + * its write path, so a static import would close an eval-order cycle. */ +async function reapArchivedBlocks( + records: CloudMemoryRecord[], + binding: CachedBinding, + directory?: string, +): Promise { + let removed = 0 + for (const record of records) { + if (!isMirrorRecord(record) || !isArchived(record)) continue + const meta = (record.metadata ?? {}) as Record + const blockId = typeof meta.block_id === "string" ? meta.block_id : undefined + const scope = meta.block_scope === "global" || meta.block_scope === "project" ? meta.block_scope : undefined + if (!blockId || !scope) continue + + // A project block belongs to one workspace AND one project; two projects in + // one workspace may hold the same block id, so both have to match or the + // reap would delete the other project's file. A global block belongs to the + // account and applies everywhere, so it needs neither. + if (scope === "project") { + if (String(meta.datamate_id ?? "") !== String(binding.datamateId)) continue + const recordProject = (meta.repo_remote as string | undefined) ?? (meta.project_path as string | undefined) + if (recordProject && recordProject !== projectKeyFor(binding)) continue + } + + let block: MemoryBlock | undefined + try { + block = syncInternals.readBlock + ? await syncInternals.readBlock(scope, blockId, directory) + : await (await import("@/memory/store")).MemoryStore.read(scope, blockId, directory) + } catch (err) { + // A read failure is not evidence the block should go. + log.warn("could not read a block to reap; leaving it", { id: blockId, scope, err: String(err) }) + continue + } + if (!block) continue + + // The archive is a point in time. A block edited after it is something the + // user wrote SINCE deciding to remove the old one, and deleting that would + // destroy work. Both are ISO-8601, so lexical order is chronological — the + // same comparison the newer-remote guard in `push` makes. + const archivedAt = typeof meta.archived_at === "string" ? meta.archived_at : undefined + if (archivedAt && block.updated > archivedAt) { + log.info("keeping a block edited after its record was archived", { + id: blockId, + scope, + blockUpdated: block.updated, + archivedAt, + }) + continue + } + + try { + const gone = syncInternals.removeBlock + ? await syncInternals.removeBlock(scope, blockId, directory) + : await (await import("@/memory/store")).MemoryStore.remove(scope, blockId, directory) + if (gone) { + removed += 1 + log.info("removed a local block whose workspace record was archived", { id: blockId, scope }) + } + } catch (err) { + log.warn("could not remove a block whose record was archived", { id: blockId, scope, err: String(err) }) + } + } + return removed +} + async function push( block: MemoryBlock, binding: CachedBinding | null, @@ -1193,6 +1283,16 @@ async function loadWorkspaceMemory(directory?: string): Promise { const ownWorkspace = String(binding.datamateId) const records = await MemoryApi.list() + // Hooked here because this is the one path that already holds the whole + // record set, a settled binding, and the directory they belong to — and it + // runs on every hydrate and refresh, so a removal made in the web app lands + // on the next session rather than waiting for a sweep. Awaited rather than + // detached: the blocks below are what this load will serve, and reaping + // after that would leave one session still reading what was just deleted. + await reapArchivedBlocks(records, binding, dir ?? undefined).catch((err) => + log.warn("could not reap archived blocks", { err: String(err) }), + ) + const blocks: RemoteMemoryBlock[] = [] for (const record of records) { if (!record?.id) continue diff --git a/packages/opencode/src/memory/store.ts b/packages/opencode/src/memory/store.ts index 7f3385d2d..4e38887fe 100644 --- a/packages/opencode/src/memory/store.ts +++ b/packages/opencode/src/memory/store.ts @@ -338,8 +338,20 @@ export namespace MemoryStore { return { duplicates } } - export async function remove(scope: "global" | "project", id: string): Promise { - const filepath = blockPath(scope, id) + /** Delete a block, and archive its workspace record. + * + * ``directory`` names the project the block belongs to. It matters because + * both halves resolve per project: without it the path and the binding come + * from the ambient instance, which for a caller acting on a project other + * than the current one deletes the wrong file and archives the wrong + * workspace's record. Callers inside a session may omit it; the reaper in + * workspace/memory-sync passes the directory it loaded records for. */ + export async function remove( + scope: "global" | "project", + id: string, + directory?: string, + ): Promise { + const filepath = blockPath(scope, id, directory) try { await fs.unlink(filepath) await appendAuditLog(scope, auditEntry("DELETE", id, scope)) @@ -359,7 +371,7 @@ export namespace MemoryStore { // deleting project's directory is captured here while its context is // still current — otherwise the archive resolves another project's // binding and can hit that workspace's same-id record. - const deletingDirectory = safeDirectory() + const deletingDirectory = directory ?? safeDirectory() void archiveBlock(scope, id, deletingDirectory).catch((e) => { mirrorLog.warn("failed to archive workspace memory record", { id, diff --git a/packages/opencode/test/altimate/workspace/memory-sync.test.ts b/packages/opencode/test/altimate/workspace/memory-sync.test.ts index 01d84a9d8..633927775 100644 --- a/packages/opencode/test/altimate/workspace/memory-sync.test.ts +++ b/packages/opencode/test/altimate/workspace/memory-sync.test.ts @@ -189,6 +189,8 @@ afterEach(() => { process.env.ALTIMATE_WORKSPACE = "1" delete syncInternals.resolveBinding delete syncInternals.blockExists + delete syncInternals.readBlock + delete syncInternals.removeBlock }) // ── record id extraction ──────────────────────────────────────────────────── @@ -977,6 +979,120 @@ describe("belongsHere", () => { }) }) +describe("reaping blocks archived elsewhere", () => { + // Removal used to be one-directional: deleting locally archived the cloud + // record, but archiving from the web app left the block on disk, where prompt + // injection kept reading it and the next edit re-created the record. These + // pin the other direction. + const archived = (blockId: string, extra: Record = {}) => ({ + id: `mem-${blockId}`, + memory: "text", + metadata: { + source: MIRROR_SOURCE, + block_id: blockId, + block_scope: "global", + archived: "true", + archived_at: "2026-06-01T00:00:00.000Z", + ...extra, + }, + }) + + const seeThrough = (blocks: Record) => { + const removed: string[] = [] + syncInternals.readBlock = async (_scope, id) => + blocks[id] ? ({ id, scope: "global", content: "c", tags: [], created: "", updated: blocks[id].updated } as any) : undefined + syncInternals.removeBlock = async (_scope, id) => { + removed.push(id) + return true + } + return removed + } + + test("a block whose record was archived elsewhere is deleted locally", async () => { + const removed = seeThrough({ gone: { updated: "2026-05-01T00:00:00.000Z" } }) + listResponse = [archived("gone")] + await refresh(`${SES}-reap-1`) + expect(removed).toEqual(["gone"]) + }) + + test("a block edited after the archive is kept", async () => { + // The user wrote this SINCE deciding to remove the old one. Deleting it + // would destroy work; the ordinary push re-mirrors it instead. + const removed = seeThrough({ newer: { updated: "2026-07-01T00:00:00.000Z" } }) + listResponse = [archived("newer")] + await refresh(`${SES}-reap-2`) + expect(removed).toEqual([]) + }) + + test("a live record is never reaped", async () => { + const removed = seeThrough({ live: { updated: "2026-05-01T00:00:00.000Z" } }) + listResponse = [ + { id: "mem-live", memory: "t", metadata: { source: MIRROR_SOURCE, block_id: "live", block_scope: "global" } }, + ] + await refresh(`${SES}-reap-3`) + expect(removed).toEqual([]) + }) + + test("another client's archived record is not reaped", async () => { + // No `source`, so it is not ours to act on even though it is archived and + // carries block-shaped metadata. + const removed = seeThrough({ imp: { updated: "2026-05-01T00:00:00.000Z" } }) + listResponse = [ + { + id: "mem-imp", + memory: "t", + metadata: { block_id: "imp", block_scope: "global", archived: "true" }, + }, + ] + await refresh(`${SES}-reap-4`) + expect(removed).toEqual([]) + }) + + test("an archived record from another workspace is not reaped", async () => { + const removed = seeThrough({ elsewhere: { updated: "2026-05-01T00:00:00.000Z" } }) + listResponse = [archived("elsewhere", { block_scope: "project", datamate_id: "7" })] + await refresh(`${SES}-reap-5`) + expect(removed).toEqual([]) + }) + + test("an archived record for another PROJECT in this workspace is not reaped", async () => { + // The workspace matches, so only the project key separates them. Two + // projects in one workspace may hold the same block id, and reaping on the + // workspace alone would delete the other project's file. + const removed = seeThrough({ shared: { updated: "2026-05-01T00:00:00.000Z" } }) + listResponse = [ + archived("shared", { + block_scope: "project", + datamate_id: String(BINDING.datamateId), + repo_remote: "ssh://git@github.com/acme/something-else.git", + }), + ] + await refresh(`${SES}-reap-5b`) + expect(removed).toEqual([]) + }) + + test("an archived record for THIS project is reaped", async () => { + // The positive half of the pair above: same workspace, same project key. + const removed = seeThrough({ ours: { updated: "2026-05-01T00:00:00.000Z" } }) + listResponse = [ + archived("ours", { + block_scope: "project", + datamate_id: String(BINDING.datamateId), + repo_remote: BINDING.repoRemote, + }), + ] + await refresh(`${SES}-reap-5c`) + expect(removed).toEqual(["ours"]) + }) + + test("a block that is already gone locally is not reported as removed", async () => { + const removed = seeThrough({}) + listResponse = [archived("absent")] + await refresh(`${SES}-reap-6`) + expect(removed).toEqual([]) + }) +}) + describe("hydrate", () => { test("keeps only this CLI's records", async () => { // The discriminating case is the last one: block-shaped metadata written by From aac99dabc79084ab8ec9aba5971ec9ff9e0c85df Mon Sep 17 00:00:00 2001 From: Sarav Date: Wed, 30 Sep 2026 08:15:02 +0530 Subject: [PATCH 3/5] fix(memory): the reaper could delete live memory; make every rule a refusal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two ways the archived-record reaper destroyed memory the user still had, both found in review. **A split-create extra took its live primary's block with it.** `push` archives the extras of a split create with the SAME identity metadata as the primary it keeps, and their `archived_at` is push time — always later than the block's `updated`. Deciding one record at a time, the extra passed every guard: the block was unlinked, and `MemoryStore.remove` then followed the index to archive the LIVE primary too. Every block split before this existed carries such an extra, so it would have fired on the first load after upgrade. The decision is now per BLOCK. A block with any live record is never reaped, however many archived ones sit beside it. **The re-archive moved the tombstone forward.** `archiveNow` prefers the index entry, so on a machine whose index named the archived record the reap re-stamped `archived_at` to now. That is the moment every other machine compares a local block against, so a block legitimately recreated after the original archive began to look older than the tombstone and was deleted everywhere — taking the recreated record with it. `archiveNow` now returns early on an already-archived record, leaving the tombstone alone. The identity search only skipped archived records for callers with no index entry; this closes the path the others took. Also, each a refusal rather than a best guess: - `archived_at` must parse. Absent or malformed, the only guard against deleting a recent edit does not exist, so the block stays. The CLI always writes it; nothing guarantees another writer does. - Compared numerically, and `>=` keeps the block: equal timestamps cannot order the two, and "cannot tell" must not delete. - The removal is conditional on the block still being what was read. `write` renames a newer file into place, and the gap between the read and the unlink is whatever runs in between; `MemoryStore.remove` takes `expectUpdated` and re-checks immediately before unlinking. Narrowed, not closed — only a lock shared with `write` closes it. - The binding is fenced per iteration. `commitLoad` drops a load whose binding changed, but that discards a RESULT, which is no help once files are gone. - Bounded at 25 blocks per load: the archived set only grows, and this runs inside the hydration budget. `MemoryStore.remove` also threads its directory to the audit entry. It reached the path and the archive but not the log, so the DELETE line went to whichever project was ambient — or, with no instance at all, threw from outside the best-effort try with the file already unlinked: no audit entry, no telemetry, and a successful delete reported as a failure. Tests, each verified by a mutation that fails it: the split-create case, an index-named tombstone, a missing and an unparseable `archived_at`, the equal-timestamp boundary, the conditional removal, and a relink mid-reap. The reaper's double now records scope, directory and expectUpdated rather than only the id — it could not have failed a wrong-directory removal before. Six more run against the REAL store, since the seams stub the removal out entirely. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Tneq4MLNoRLsX7htV6fGZC --- .../src/altimate/workspace/memory-sync.ts | 144 +++++++++++++---- packages/opencode/src/memory/store.ts | 44 +++++- .../altimate/workspace/memory-sync.test.ts | 147 ++++++++++++++++-- .../test/memory/store-directory.test.ts | 54 +++++++ 4 files changed, 337 insertions(+), 52 deletions(-) diff --git a/packages/opencode/src/altimate/workspace/memory-sync.ts b/packages/opencode/src/altimate/workspace/memory-sync.ts index 254fa0be5..01d4689d2 100644 --- a/packages/opencode/src/altimate/workspace/memory-sync.ts +++ b/packages/opencode/src/altimate/workspace/memory-sync.ts @@ -121,7 +121,12 @@ export const syncInternals: { /** Test seams for the archived-record reaper. Production reads and writes the * store; both are here so a test can observe a removal without a real file. */ readBlock?: (scope: "global" | "project", id: string, directory?: string) => Promise - removeBlock?: (scope: "global" | "project", id: string, directory?: string) => Promise + removeBlock?: ( + scope: "global" | "project", + id: string, + directory?: string, + expectUpdated?: string, + ) => Promise } = {} /** Instance.directory throws synchronously with no instance context, so a @@ -446,6 +451,11 @@ async function existsLocally(block: MemoryBlock, directory?: string): Promise boolean, ): Promise { - let removed = 0 + const archived: { record: CloudMemoryRecord; scope: "global" | "project"; blockId: string }[] = [] for (const record of records) { - if (!isMirrorRecord(record) || !isArchived(record)) continue + if (!isMirrorRecord(record)) continue const meta = (record.metadata ?? {}) as Record const blockId = typeof meta.block_id === "string" ? meta.block_id : undefined const scope = meta.block_scope === "global" || meta.block_scope === "project" ? meta.block_scope : undefined if (!blockId || !scope) continue - - // A project block belongs to one workspace AND one project; two projects in - // one workspace may hold the same block id, so both have to match or the - // reap would delete the other project's file. A global block belongs to the - // account and applies everywhere, so it needs neither. if (scope === "project") { if (String(meta.datamate_id ?? "") !== String(binding.datamateId)) continue const recordProject = (meta.repo_remote as string | undefined) ?? (meta.project_path as string | undefined) if (recordProject && recordProject !== projectKeyFor(binding)) continue } + if (isArchived(record)) archived.push({ record, scope, blockId }) + } + if (archived.length === 0) return 0 + + /** Block identities with a live record. `isSameBlock` already excludes + * archived records, so this is exactly the set that must not be reaped. */ + const live = new Set() + for (const { scope, blockId } of archived) { + const key = `${scope}:${blockId}` + if (live.has(key)) continue + const stub = { id: blockId, scope, tags: [], content: "", created: "", updated: "" } as MemoryBlock + if (records.some((r) => isSameBlock(r, stub, binding))) live.add(key) + } + + let removed = 0 + const seen = new Set() + for (const { record, scope, blockId } of archived) { + if (removed >= REAP_LIMIT_PER_LOAD) break + // A relink can land mid-loop. `commitLoad` drops the load's RESULT on an + // epoch change, which is no help once files are gone. (review) + if (!stillCurrent()) { + log.info("stopping the reap: the binding changed while it ran", { directory }) + break + } + const key = `${scope}:${blockId}` + if (seen.has(key)) continue + seen.add(key) + if (live.has(key)) continue + + // Parse before reading the block: an unusable tombstone costs no file I/O, + // and the archived set only grows. + const rawArchivedAt = (record.metadata ?? {})["archived_at"] + const archivedMs = typeof rawArchivedAt === "string" ? Date.parse(rawArchivedAt) : Number.NaN + if (Number.isNaN(archivedMs)) { + log.warn("archived record has no usable archived_at; leaving the block", { id: blockId, scope }) + continue + } let block: MemoryBlock | undefined try { block = syncInternals.readBlock - ? await syncInternals.readBlock(scope, blockId, directory) - : await (await import("@/memory/store")).MemoryStore.read(scope, blockId, directory) + ? await syncInternals.readBlock(scope, blockId, directory ?? undefined) + : await (await import("@/memory/store")).MemoryStore.read(scope, blockId, directory ?? undefined) } catch (err) { - // A read failure is not evidence the block should go. log.warn("could not read a block to reap; leaving it", { id: blockId, scope, err: String(err) }) continue } if (!block) continue - // The archive is a point in time. A block edited after it is something the - // user wrote SINCE deciding to remove the old one, and deleting that would - // destroy work. Both are ISO-8601, so lexical order is chronological — the - // same comparison the newer-remote guard in `push` makes. - const archivedAt = typeof meta.archived_at === "string" ? meta.archived_at : undefined - if (archivedAt && block.updated > archivedAt) { - log.info("keeping a block edited after its record was archived", { + // `>=` keeps the block on a tie: equal timestamps cannot order the two, and + // the safe reading of "cannot tell" is to keep what the user has. + const blockMs = Date.parse(block.updated) + if (Number.isNaN(blockMs) || blockMs >= archivedMs) { + log.info("keeping a block not older than its record's archive", { id: blockId, scope, blockUpdated: block.updated, - archivedAt, + archivedAt: rawArchivedAt, }) continue } try { const gone = syncInternals.removeBlock - ? await syncInternals.removeBlock(scope, blockId, directory) - : await (await import("@/memory/store")).MemoryStore.remove(scope, blockId, directory) + ? await syncInternals.removeBlock(scope, blockId, directory ?? undefined, block.updated) + : await (await import("@/memory/store")).MemoryStore.remove(scope, blockId, directory ?? undefined, { + expectUpdated: block.updated, + }) if (gone) { removed += 1 log.info("removed a local block whose workspace record was archived", { id: blockId, scope }) @@ -825,6 +880,28 @@ async function archiveNow( return } + // Already a tombstone: leave it exactly as it is. + // + // Not merely a saved request. `archived_at` is read as the moment the record + // stopped being wanted, and the reaper on another machine compares a local + // block's `updated` against it. Re-stamping it to now moves that moment + // forward, so a block legitimately recreated AFTER the original archive + // starts looking older than the tombstone and gets deleted — on every + // machine, taking the recreated record with it, because this function then + // follows the index to whatever the block points at now. + // + // The identity search below already skips archived records, so a caller with + // no index entry lands here anyway. This closes the path a caller WITH one + // takes. (review) + if (isArchived(current)) { + log.info("record is already archived; leaving its tombstone untouched", { + blockId, + scope, + memoryId: current.id, + }) + return + } + const now = new Date().toISOString() const metadata: MirrorMetadata = { ...((current.metadata ?? {}) as unknown as MirrorMetadata), @@ -1289,7 +1366,12 @@ async function loadWorkspaceMemory(directory?: string): Promise { // on the next session rather than waiting for a sweep. Awaited rather than // detached: the blocks below are what this load will serve, and reaping // after that would leave one session still reading what was just deleted. - await reapArchivedBlocks(records, binding, dir ?? undefined).catch((err) => + // + // Fenced on the binding epoch. `commitLoad` already drops a load whose + // binding changed underneath it, but that discards a RESULT — no help once + // files have been unlinked. The fence is re-read per iteration inside, so a + // relink landing mid-loop stops the rest. (review) + await reapArchivedBlocks(records, binding, dir, () => epochFor(dir) === epoch).catch((err) => log.warn("could not reap archived blocks", { err: String(err) }), ) diff --git a/packages/opencode/src/memory/store.ts b/packages/opencode/src/memory/store.ts index 4e38887fe..d6b2b7f46 100644 --- a/packages/opencode/src/memory/store.ts +++ b/packages/opencode/src/memory/store.ts @@ -66,8 +66,8 @@ function blockPath(scope: "global" | "project", id: string, directory?: string): return result } -function auditLogPath(scope: "global" | "project"): string { - return path.join(dirForScope(scope), ".log") +function auditLogPath(scope: "global" | "project", directory?: string): string { + return path.join(dirForScope(scope, directory), ".log") } function parseFrontmatter(raw: string): { meta: Record; content: string } | undefined { @@ -117,11 +117,20 @@ export function isExpired(block: MemoryBlock): boolean { return new Date(block.expires) <= new Date() } -async function appendAuditLog(scope: "global" | "project", entry: string): Promise { - const logPath = auditLogPath(scope) - const dir = path.dirname(logPath) +async function appendAuditLog( + scope: "global" | "project", + entry: string, + directory?: string, +): Promise { + // Inside the try, not above it. `auditLogPath` resolves project scope through + // the ambient instance when no directory is given, and `Instance.directory` + // THROWS where there is none — the headless refresh path has none. Resolved + // outside, that rejection escaped a function whose whole contract is to be + // best-effort, and reached a caller that had already unlinked the file: no + // audit entry, no telemetry, and a delete reported as a failure. try { - await fs.mkdir(dir, { recursive: true }) + const logPath = auditLogPath(scope, directory) + await fs.mkdir(path.dirname(logPath), { recursive: true }) await fs.appendFile(logPath, entry + "\n", "utf-8") } catch { // Audit logging is best-effort — never fail the operation @@ -350,11 +359,32 @@ export namespace MemoryStore { scope: "global" | "project", id: string, directory?: string, + opts?: { expectUpdated?: string }, ): Promise { const filepath = blockPath(scope, id, directory) try { + // A caller that decided to delete based on a value it read earlier states + // that value here, and the block is re-read against it immediately before + // the unlink. `write` renames a new file into place, so an edit landing + // between another caller's read and this unlink would otherwise be + // deleted — the caller cannot close that itself, because the gap is + // whatever runs in between. Narrowed to this function; only a lock shared + // with `write` would close it outright. + if (opts?.expectUpdated !== undefined) { + const current = await read(scope, id, directory) + if (!current) return false + if (current.updated !== opts.expectUpdated) { + mirrorLog.info("not removing a block that changed after it was checked", { + id, + scope, + expected: opts.expectUpdated, + actual: current.updated, + }) + return false + } + } await fs.unlink(filepath) - await appendAuditLog(scope, auditEntry("DELETE", id, scope)) + await appendAuditLog(scope, auditEntry("DELETE", id, scope), directory) Telemetry.track({ type: "memory_operation", timestamp: Date.now(), diff --git a/packages/opencode/test/altimate/workspace/memory-sync.test.ts b/packages/opencode/test/altimate/workspace/memory-sync.test.ts index 633927775..62db8d772 100644 --- a/packages/opencode/test/altimate/workspace/memory-sync.test.ts +++ b/packages/opencode/test/altimate/workspace/memory-sync.test.ts @@ -997,22 +997,28 @@ describe("reaping blocks archived elsewhere", () => { }, }) - const seeThrough = (blocks: Record) => { - const removed: string[] = [] - syncInternals.readBlock = async (_scope, id) => - blocks[id] ? ({ id, scope: "global", content: "c", tags: [], created: "", updated: blocks[id].updated } as any) : undefined - syncInternals.removeBlock = async (_scope, id) => { - removed.push(id) + /** Records the arguments a removal was asked for, not just that one happened: + * the scope and directory decide WHICH file goes, and a stub that drops them + * cannot fail a wrong-directory removal. (review) */ + const seeThrough = (blocks: Record) => { + const removed: { id: string; scope: string; directory?: string; expectUpdated?: string }[] = [] + syncInternals.readBlock = async (scope, id) => + blocks[id] + ? ({ id, scope, content: "c", tags: [], created: "", updated: blocks[id].updated } as any) + : undefined + syncInternals.removeBlock = async (scope, id, directory, expectUpdated) => { + removed.push({ id, scope, directory, expectUpdated }) return true } return removed } + const idsOf = (removed: { id: string }[]) => removed.map((r) => r.id) test("a block whose record was archived elsewhere is deleted locally", async () => { const removed = seeThrough({ gone: { updated: "2026-05-01T00:00:00.000Z" } }) listResponse = [archived("gone")] await refresh(`${SES}-reap-1`) - expect(removed).toEqual(["gone"]) + expect(idsOf(removed)).toEqual(["gone"]) }) test("a block edited after the archive is kept", async () => { @@ -1021,7 +1027,7 @@ describe("reaping blocks archived elsewhere", () => { const removed = seeThrough({ newer: { updated: "2026-07-01T00:00:00.000Z" } }) listResponse = [archived("newer")] await refresh(`${SES}-reap-2`) - expect(removed).toEqual([]) + expect(idsOf(removed)).toEqual([]) }) test("a live record is never reaped", async () => { @@ -1030,7 +1036,7 @@ describe("reaping blocks archived elsewhere", () => { { id: "mem-live", memory: "t", metadata: { source: MIRROR_SOURCE, block_id: "live", block_scope: "global" } }, ] await refresh(`${SES}-reap-3`) - expect(removed).toEqual([]) + expect(idsOf(removed)).toEqual([]) }) test("another client's archived record is not reaped", async () => { @@ -1045,14 +1051,14 @@ describe("reaping blocks archived elsewhere", () => { }, ] await refresh(`${SES}-reap-4`) - expect(removed).toEqual([]) + expect(idsOf(removed)).toEqual([]) }) test("an archived record from another workspace is not reaped", async () => { const removed = seeThrough({ elsewhere: { updated: "2026-05-01T00:00:00.000Z" } }) listResponse = [archived("elsewhere", { block_scope: "project", datamate_id: "7" })] await refresh(`${SES}-reap-5`) - expect(removed).toEqual([]) + expect(idsOf(removed)).toEqual([]) }) test("an archived record for another PROJECT in this workspace is not reaped", async () => { @@ -1068,7 +1074,7 @@ describe("reaping blocks archived elsewhere", () => { }), ] await refresh(`${SES}-reap-5b`) - expect(removed).toEqual([]) + expect(idsOf(removed)).toEqual([]) }) test("an archived record for THIS project is reaped", async () => { @@ -1082,14 +1088,104 @@ describe("reaping blocks archived elsewhere", () => { }), ] await refresh(`${SES}-reap-5c`) - expect(removed).toEqual(["ours"]) + expect(idsOf(removed)).toEqual(["ours"]) + }) + + test("a split-create extra does not take its live primary's block with it", async () => { + // `push` archives the extras of a split create with the SAME identity + // metadata as the primary it keeps, and their archived_at is push time, + // always later than the block's updated. Deciding per record, the extra + // looked exactly like a tombstone — so the block was unlinked and the + // removal then followed the index and archived the live primary too. Every + // block split before this existed carries such an extra, so this fired on + // the first load after upgrade. (review) + const removed = seeThrough({ split: { updated: "2026-05-01T00:00:00.000Z" } }) + listResponse = [ + // The live primary. + { id: "mem-primary", memory: "kept", metadata: { source: MIRROR_SOURCE, block_id: "split", block_scope: "global" } }, + // The extra, archived at push time. + archived("split", { archived_at: "2026-06-01T00:00:00.000Z" }), + ] + await refresh(`${SES}-split`) + expect(idsOf(removed)).toEqual([]) + }) + + test("an archived record with no usable archived_at leaves the block alone", async () => { + // Absent, the only guard against deleting a recent edit does not exist. The + // CLI always writes it; nothing guarantees another writer does. (review) + const removed = seeThrough({ nostamp: { updated: "2026-05-01T00:00:00.000Z" } }) + listResponse = [ + { + id: "mem-nostamp", + memory: "t", + metadata: { source: MIRROR_SOURCE, block_id: "nostamp", block_scope: "global", archived: "true" }, + }, + ] + await refresh(`${SES}-nostamp`) + expect(idsOf(removed)).toEqual([]) + }) + + test("an unparseable archived_at leaves the block alone", async () => { + const removed = seeThrough({ junk: { updated: "2026-05-01T00:00:00.000Z" } }) + listResponse = [archived("junk", { archived_at: "not a date" })] + await refresh(`${SES}-junk`) + expect(idsOf(removed)).toEqual([]) + }) + + test("a block updated at exactly the archive time is kept", async () => { + // Equal timestamps cannot order the two, and the safe reading of "cannot + // tell" is to keep what the user has. (review) + const removed = seeThrough({ tie: { updated: "2026-06-01T00:00:00.000Z" } }) + listResponse = [archived("tie")] + await refresh(`${SES}-tie`) + expect(idsOf(removed)).toEqual([]) + }) + + test("the removal is conditional on the block still being what was read", async () => { + // The gap between the read and the unlink is whatever runs in between, so + // the caller states what it saw and the store re-checks it. (review) + const removed = seeThrough({ cond: { updated: "2026-05-01T00:00:00.000Z" } }) + listResponse = [archived("cond")] + await refresh(`${SES}-cond`) + expect(removed).toHaveLength(1) + expect(removed[0].expectUpdated).toBe("2026-05-01T00:00:00.000Z") + expect(removed[0].scope).toBe("global") + }) + + test("a relink landing mid-reap stops the rest", async () => { + // `commitLoad` drops the load's RESULT on an epoch change, which is no help + // once files are gone. (review) + const { recordApprovedBinding } = await import("../../../src/altimate/workspace/state") + const removed = seeThrough({ + first: { updated: "2026-05-01T00:00:00.000Z" }, + second: { updated: "2026-05-01T00:00:00.000Z" }, + }) + const here = mkdtempSync(path.join(SANDBOX, "reap-fence-")) + // Relink as soon as the first block is read, so the fence is false by the + // time the loop comes round again. + let relinked = false + const inner = syncInternals.readBlock! + syncInternals.readBlock = async (scope, id, directory) => { + if (!relinked) { + relinked = true + await recordApprovedBinding( + here, + { datamateId: 99, datamateName: "other", repoRemote: null, projectPath: here, linkedAt: Date.now() }, + { seed: false }, + ) + } + return inner(scope, id, directory) + } + listResponse = [archived("first"), archived("second")] + await refresh(`${SES}-fence`, here) + expect(idsOf(removed).length).toBeLessThan(2) }) test("a block that is already gone locally is not reported as removed", async () => { const removed = seeThrough({}) listResponse = [archived("absent")] await refresh(`${SES}-reap-6`) - expect(removed).toEqual([]) + expect(idsOf(removed)).toEqual([]) }) }) @@ -1317,6 +1413,29 @@ describe("archiveBlock", () => { await archiveBlock("global", "done") expect(captured.filter((c) => c.method === "PATCH").length).toBe(0) }) + + test("an already-archived record the INDEX names is not re-stamped either", async () => { + // The identity search skips archived records, so the test above only covers + // a machine with no index entry. A machine that HAS one reached the record + // directly and re-stamped `archived_at` to now — moving the moment the + // record stopped being wanted, which is what every other machine's + // edited-after-archive guard compares against. A block legitimately + // recreated after the original archive then looks older than the tombstone + // and is deleted everywhere. (review) + const b = block({ id: "indexed-tomb" }) + createResult = [{ id: "mem-indexed-tomb" }] + await mirrorBlock(b) + + listResponse = listResponse.map((r: any) => + r.id === "mem-indexed-tomb" + ? { ...r, metadata: { ...r.metadata, archived: "true", archived_at: "2026-06-01T00:00:00.000Z" } } + : r, + ) + captured = [] + await archiveBlock("global", "indexed-tomb") + + expect(captured.filter((c) => c.method === "PATCH").length).toBe(0) + }) }) describe("backfill", () => { diff --git a/packages/opencode/test/memory/store-directory.test.ts b/packages/opencode/test/memory/store-directory.test.ts index 26d19e1d7..70d5dae70 100644 --- a/packages/opencode/test/memory/store-directory.test.ts +++ b/packages/opencode/test/memory/store-directory.test.ts @@ -62,4 +62,58 @@ describe("MemoryStore project scope with an explicit directory", () => { expect(Array.isArray(blocks)).toBe(true) expect(blocks.every((b) => b.scope === "global")).toBe(true) }) + + // `remove` deletes a file, so its directory has to reach every step. These + // run against the real store on purpose: the reaper's test seams stub the + // removal out, so a wrong-directory unlink or a missing audit entry would + // pass there. (review) + test("remove with an explicit directory deletes that project's file and logs there", async () => { + const removed = await MemoryStore.remove("project", "proj-block", proj) + expect(removed).toBe(true); + + await expect( + fs.access(path.join(proj, ".altimate-code", "memory", "proj-block.md")) + ).rejects.toThrow() + + // The audit entry lands beside the file it describes. Resolved from the + // ambient instance instead, it was written to whichever project happened to + // be current — or threw, with the file already gone. + const log = await fs.readFile(path.join(proj, ".altimate-code", "memory", ".log"), "utf-8") + expect(log).toContain("DELETE project/proj-block") + }) + + test("remove with no instance context still records the deletion", async () => { + // The headless refresh path has no instance. `auditLogPath` resolves project + // scope through `Instance.directory`, which THROWS there — and it used to be + // called outside the best-effort try, so the rejection escaped a function + // that had already unlinked the file. (review) + await expect(MemoryStore.remove("project", "proj-block", proj)).resolves.toBe(true) + const log = await fs.readFile(path.join(proj, ".altimate-code", "memory", ".log"), "utf-8") + expect(log).toContain("DELETE project/proj-block") + }) + + test("remove keeps a block that changed after the caller read it", async () => { + // The caller decided from a value it read earlier; `write` renames a newer + // file into place. Without the condition the newer edit is deleted. + const stale = "2020-01-01T00:00:00.000Z" + const removed = await MemoryStore.remove("project", "proj-block", proj, { expectUpdated: stale }) + + expect(removed).toBe(false) + const block = await MemoryStore.read("project", "proj-block", proj) + expect(block?.content).toBe("A project fact.") + }) + + test("remove proceeds when the block is still what the caller read", async () => { + const before = await MemoryStore.read("project", "proj-block", proj) + const removed = await MemoryStore.remove("project", "proj-block", proj, { + expectUpdated: before!.updated, + }) + + expect(removed).toBe(true) + expect(await MemoryStore.read("project", "proj-block", proj)).toBeUndefined() + }) + + test("remove reports false for a block that is already gone", async () => { + expect(await MemoryStore.remove("project", "no-such-block", proj)).toBe(false) + }) }) From d70d35ba2367436b8b1ff4311267d1a0f4f83315 Mon Sep 17 00:00:00 2001 From: Sarav Date: Wed, 30 Sep 2026 08:16:28 +0530 Subject: [PATCH 4/5] test(memory): pin the index rewrite and the unchanged-block limit Both asked for in review. `a record deleted out from under us` asserted the POST but not that the stale entry was rewritten, leaving the self-heal half-proven: an entry still naming the dead id would go on failing on every later save. A third save now has to PATCH the new record. `an UNCHANGED block is not re-created when its record disappears` pins the limit rather than implying the block returns on its own. `push` returns at the content-hash guard for a block whose payload is already indexed, so nothing notices the record is gone until the block is edited. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Tneq4MLNoRLsX7htV6fGZC --- .../altimate/workspace/memory-sync.test.ts | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/packages/opencode/test/altimate/workspace/memory-sync.test.ts b/packages/opencode/test/altimate/workspace/memory-sync.test.ts index 62db8d772..c2151d701 100644 --- a/packages/opencode/test/altimate/workspace/memory-sync.test.ts +++ b/packages/opencode/test/altimate/workspace/memory-sync.test.ts @@ -696,6 +696,34 @@ describe("mirrorBlock", () => { expect(callsTo("/datamates/memory/mem-gone", "PATCH").length).toBe(0) expect(callsTo("/datamates/memory/", "POST").length).toBe(1) + + // And the index now names the record that exists. Asserting only the POST + // leaves the self-heal half-proven: an entry still pointing at the dead id + // would go on failing on every later save. A third save has to reach + // `mem-fresh`. (review) + captured = [] + await mirrorBlock({ ...b, content: "third", updated: "2028-01-01T00:00:00.000Z" }) + expect(callsTo("/datamates/memory/mem-fresh", "PATCH").length).toBe(1) + expect(callsTo("/datamates/memory/mem-gone", "PATCH").length).toBe(0) + }) + + test("an UNCHANGED block is not re-created when its record disappears", async () => { + // The self-heal runs on the next SAVE, not on the next load: `push` returns + // at the content-hash guard for a block whose payload is already indexed, + // so nothing notices the record has gone until the block is edited. This + // pins the behaviour as it is rather than implying the block comes back on + // its own — recovering it would mean invalidating index entries a complete + // load shows to be absent, which this change does not do. (review) + const b = block({ id: "untouched" }) + createResult = [{ id: "mem-untouched" }] + await mirrorBlock(b) + + listResponse = listResponse.filter((r: any) => r.id !== "mem-untouched") + captured = [] + await mirrorBlock(b) + + expect(callsTo("/datamates/memory/", "POST").length).toBe(0) + expect(captured.length).toBe(0) }) test("a truncated read does not mistake an unreachable record for a deleted one", async () => { From 386ca266a546816c5fc15143e702318d3c3fb861 Mon Sep 17 00:00:00 2001 From: Sarav Date: Wed, 30 Sep 2026 10:44:25 +0530 Subject: [PATCH 5/5] fix(memory): a partial listing proves nothing, and the newest tombstone decides MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three findings from the review bots on the reaper, all real. The truncation hole is the same bug as the split-create one, back through a door I left open. The live-record test is what stops an archived extra taking its live primary's block with it — and that test is only as good as the window it ran on. `loadWorkspaceMemory` passed a bare `MemoryApi.list()` to the reaper, so a response at exactly `LIST_LIMIT` holding an extra whose primary sits outside the window reads as a dead block, and a live memory's file is deleted. The load now reads through `fetchKnownRecords`, which already knows how to spot that, and the reaper refuses to act on a partial view at all. The tombstone choice was order-dependent. `seen` was marked before the timestamp was parsed or compared, so the first archived record the service happened to list for a block was the only one ever considered. With a stale extra ahead of the primary's later archive, the block was kept against the old timestamp and the real removal was never reconsidered — not on that load, nor on any later one with the same ordering. Tombstones are now grouped by block and the newest parseable `archived_at` decides. The live set was built with a `records.some()` scan per archived identity — quadratic on every hydrate and refresh, synchronous, and on a list that only grows. Both sets are now built in one pass. That pass needed the identity rules the reaper had been carrying as its own copy beside `isSameBlock`'s, which is how a rule drifts: one copy gets a fix and the other does not. They are now one function, `mirrorIdentityOf`, with `isSameBlock` expressed in terms of it. Reverting its project-key test fails the reaper's tests AND `archiveBlock`'s, which is the point. Five tests, each shown to fail when its guard is reverted: the truncated listing (paired with a one-record-smaller set that IS reaped, so the refusal is the truncation and nothing else), newest-wins in both directions, and a live record for another project not protecting this one's block. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Tneq4MLNoRLsX7htV6fGZC --- .../src/altimate/workspace/memory-sync.ts | 134 ++++++++++++------ .../altimate/workspace/memory-sync.test.ts | 75 ++++++++++ 2 files changed, 162 insertions(+), 47 deletions(-) diff --git a/packages/opencode/src/altimate/workspace/memory-sync.ts b/packages/opencode/src/altimate/workspace/memory-sync.ts index 01d4689d2..3cd70490f 100644 --- a/packages/opencode/src/altimate/workspace/memory-sync.ts +++ b/packages/opencode/src/altimate/workspace/memory-sync.ts @@ -384,15 +384,33 @@ export function buildMetadata(block: MemoryBlock, binding: CachedBinding | null) * Matches on logical identity only. Content is deliberately excluded: an * identity that moved when content changed could never find the record it * means to update. */ -function isSameBlock(record: CloudMemoryRecord, block: MemoryBlock, binding: CachedBinding | null): boolean { - if (!isMirrorRecord(record) || isArchived(record)) return false +/** Which local block this record mirrors, as ``scope:blockId``, or undefined when + * it mirrors none of ours. + * + * Archive state is deliberately NOT part of it: a tombstone names the same block + * its live record did, and the reaper needs both answers from the same rule. + * Callers that only want live records test ``isArchived`` themselves. + * + * This is the single expression of the matching rules. The reaper used to carry + * its own copy of the workspace/project tests beside `isSameBlock`'s, which is + * how a rule drifts: one of them gets a fix and the other does not. */ +function mirrorIdentityOf(record: CloudMemoryRecord, binding: CachedBinding | null): string | undefined { + if (!isMirrorRecord(record)) return undefined const m = record.metadata ?? {} - if (m.block_id !== block.id) return false - if (m.block_scope !== block.scope) return false - if (block.scope !== "project") return true - if (String(m.datamate_id ?? "") !== String(binding?.datamateId ?? "")) return false - const recordProject = (m.repo_remote as string | undefined) ?? (m.project_path as string | undefined) - return !recordProject || !binding || recordProject === projectKeyFor(binding) + const blockId = typeof m.block_id === "string" ? m.block_id : undefined + const scope = m.block_scope === "global" || m.block_scope === "project" ? m.block_scope : undefined + if (!blockId || !scope) return undefined + if (scope === "project") { + if (String(m.datamate_id ?? "") !== String(binding?.datamateId ?? "")) return undefined + const recordProject = (m.repo_remote as string | undefined) ?? (m.project_path as string | undefined) + if (recordProject && binding && recordProject !== projectKeyFor(binding)) return undefined + } + return `${scope}:${blockId}` +} + +function isSameBlock(record: CloudMemoryRecord, block: MemoryBlock, binding: CachedBinding | null): boolean { + if (isArchived(record)) return false + return mirrorIdentityOf(record, binding) === `${block.scope}:${block.id}` } /** Fetch the record set once, and report whether it was cut short. @@ -487,44 +505,75 @@ const REAP_LIMIT_PER_LOAD = 25 * edit landing in between is not destroyed. * - The binding must not have changed under us, checked per iteration: a relink * mid-loop means these records describe a workspace this directory has left. + * - A truncated listing reaps nothing at all. The live-record test above is only + * as good as the window it ran on, and a window that omits a live primary + * while holding its archived extra reads exactly like a dead block. (review) + * - Where a block carries several tombstones, the NEWEST parseable one decides. + * First-seen let a stale extra out-vote the primary's later archive. (review) * * Imported lazily, like `existsLocally`: `@/memory/store` reaches this module on * its write path, so a static import would close an eval-order cycle. */ async function reapArchivedBlocks( - records: CloudMemoryRecord[], + known: KnownRecords, binding: CachedBinding, directory: string | null, stillCurrent: () => boolean, ): Promise { - const archived: { record: CloudMemoryRecord; scope: "global" | "project"; blockId: string }[] = [] - for (const record of records) { - if (!isMirrorRecord(record)) continue - const meta = (record.metadata ?? {}) as Record - const blockId = typeof meta.block_id === "string" ? meta.block_id : undefined - const scope = meta.block_scope === "global" || meta.block_scope === "project" ? meta.block_scope : undefined - if (!blockId || !scope) continue - if (scope === "project") { - if (String(meta.datamate_id ?? "") !== String(binding.datamateId)) continue - const recordProject = (meta.repo_remote as string | undefined) ?? (meta.project_path as string | undefined) - if (recordProject && recordProject !== projectKeyFor(binding)) continue - } - if (isArchived(record)) archived.push({ record, scope, blockId }) + // A cut-short listing cannot answer "does this block still have a live + // record?" — and that is the whole of the split-create defence below. A window + // holding an archived extra but not its live primary would report the block as + // dead and delete a file whose memory is in use. Nothing here is urgent enough + // to run on a view we know is partial. (review) + if (known.truncated) { + log.warn("skipping the reap: the record listing is truncated, so a live record may be out of reach", { + limit: LIST_LIMIT, + }) + return 0 } - if (archived.length === 0) return 0 + const records = known.records - /** Block identities with a live record. `isSameBlock` already excludes - * archived records, so this is exactly the set that must not be reaped. */ + /** Block identities with a LIVE record, in one pass. These must never be + * reaped, whatever tombstones they also carry. */ const live = new Set() - for (const { scope, blockId } of archived) { - const key = `${scope}:${blockId}` - if (live.has(key)) continue - const stub = { id: blockId, scope, tags: [], content: "", created: "", updated: "" } as MemoryBlock - if (records.some((r) => isSameBlock(r, stub, binding))) live.add(key) + /** The tombstone that decides each block: the NEWEST parseable ``archived_at`` + * among that block's archived records. + * + * Newest, not first-seen. A split create archives its extras at push time, so + * one block can carry several tombstones with different timestamps, and the + * listing's order is the service's, not ours. Deciding on whichever arrived + * first meant an old extra could out-vote the primary's later archive: the + * block was kept against the stale timestamp and the real removal was never + * reconsidered, on that load or any later one with the same ordering. (review) */ + const tombstones = new Map() + for (const record of records) { + const key = mirrorIdentityOf(record, binding) + if (!key) continue + if (!isArchived(record)) { + live.add(key) + continue + } + // Parse here, before any file I/O: an unusable tombstone is not evidence of + // anything, and a block whose every tombstone is unusable never enters the + // map — which is the refusal, not an oversight. + const rawArchivedAt = (record.metadata ?? {})["archived_at"] + const archivedMs = typeof rawArchivedAt === "string" ? Date.parse(rawArchivedAt) : Number.NaN + if (Number.isNaN(archivedMs)) { + log.warn("archived record has no usable archived_at; it decides nothing", { key }) + continue + } + const [scope, ...rest] = key.split(":") + const prior = tombstones.get(key) + if (!prior || archivedMs > prior.archivedMs) + tombstones.set(key, { + scope: scope as "global" | "project", + blockId: rest.join(":"), + archivedMs, + }) } + if (tombstones.size === 0) return 0 let removed = 0 - const seen = new Set() - for (const { record, scope, blockId } of archived) { + for (const [key, { scope, blockId, archivedMs }] of tombstones) { if (removed >= REAP_LIMIT_PER_LOAD) break // A relink can land mid-loop. `commitLoad` drops the load's RESULT on an // epoch change, which is no help once files are gone. (review) @@ -532,20 +581,8 @@ async function reapArchivedBlocks( log.info("stopping the reap: the binding changed while it ran", { directory }) break } - const key = `${scope}:${blockId}` - if (seen.has(key)) continue - seen.add(key) if (live.has(key)) continue - // Parse before reading the block: an unusable tombstone costs no file I/O, - // and the archived set only grows. - const rawArchivedAt = (record.metadata ?? {})["archived_at"] - const archivedMs = typeof rawArchivedAt === "string" ? Date.parse(rawArchivedAt) : Number.NaN - if (Number.isNaN(archivedMs)) { - log.warn("archived record has no usable archived_at; leaving the block", { id: blockId, scope }) - continue - } - let block: MemoryBlock | undefined try { block = syncInternals.readBlock @@ -565,7 +602,7 @@ async function reapArchivedBlocks( id: blockId, scope, blockUpdated: block.updated, - archivedAt: rawArchivedAt, + archivedAt: new Date(archivedMs).toISOString(), }) continue } @@ -1358,7 +1395,10 @@ async function loadWorkspaceMemory(directory?: string): Promise { const ownProjectKey = projectKeyFor(binding) const ownWorkspace = String(binding.datamateId) - const records = await MemoryApi.list() + // `fetchKnownRecords`, not a bare `list()`: the reap below refuses to act on + // a truncated view, and it can only refuse if it is told. (review) + const known = await fetchKnownRecords() + const records = known.records // Hooked here because this is the one path that already holds the whole // record set, a settled binding, and the directory they belong to — and it @@ -1371,7 +1411,7 @@ async function loadWorkspaceMemory(directory?: string): Promise { // binding changed underneath it, but that discards a RESULT — no help once // files have been unlinked. The fence is re-read per iteration inside, so a // relink landing mid-loop stops the rest. (review) - await reapArchivedBlocks(records, binding, dir, () => epochFor(dir) === epoch).catch((err) => + await reapArchivedBlocks(known, binding, dir, () => epochFor(dir) === epoch).catch((err) => log.warn("could not reap archived blocks", { err: String(err) }), ) diff --git a/packages/opencode/test/altimate/workspace/memory-sync.test.ts b/packages/opencode/test/altimate/workspace/memory-sync.test.ts index c2151d701..934155663 100644 --- a/packages/opencode/test/altimate/workspace/memory-sync.test.ts +++ b/packages/opencode/test/altimate/workspace/memory-sync.test.ts @@ -1215,6 +1215,81 @@ describe("reaping blocks archived elsewhere", () => { await refresh(`${SES}-reap-6`) expect(idsOf(removed)).toEqual([]) }) + + test("a truncated listing reaps nothing, because it cannot prove a record is dead", async () => { + // The live-record test is only as good as the window it ran on. A window at + // the limit may hold an archived split-create extra while its live primary + // sits outside — which reads exactly like a dead block, and deletes a file + // whose memory is still in use. Same class as the split-create bug, back + // through the truncation door. (review) + const removed = seeThrough({ cut: { updated: "2026-05-01T00:00:00.000Z" } }) + const filler = Array.from({ length: LIST_LIMIT - 1 }, (_, i) => ({ + id: `mem-filler-${i}`, + memory: "t", + metadata: { source: MIRROR_SOURCE, block_id: `filler-${i}`, block_scope: "global" }, + })) + listResponse = [archived("cut"), ...filler] + expect(listResponse).toHaveLength(LIST_LIMIT) + await refresh(`${SES}-truncated`) + expect(idsOf(removed)).toEqual([]) + + // One record fewer is a complete view, and the same tombstone is acted on — + // so the refusal above is the truncation, not something else about the set. + const stillThere = seeThrough({ cut: { updated: "2026-05-01T00:00:00.000Z" } }) + listResponse = [archived("cut"), ...filler.slice(1)] + await refresh(`${SES}-not-truncated`) + expect(idsOf(stillThere)).toEqual(["cut"]) + }) + + test("the NEWEST tombstone decides, not whichever the service listed first", async () => { + // A split create archives its extras at push time, so one block can carry + // several tombstones. Deciding on the first seen let a stale extra out-vote + // the primary's later archive: the block was kept against the old timestamp + // and the real removal was never reconsidered, on this load or any later one + // with the same ordering. Here T1 < block.updated < T3, no live record left. + const removed = seeThrough({ multi: { updated: "2026-06-15T00:00:00.000Z" } }) + listResponse = [ + archived("multi", { archived_at: "2026-06-01T00:00:00.000Z" }), // the stale extra, first + archived("multi", { archived_at: "2026-07-01T00:00:00.000Z" }), // the primary's real archive + ] + await refresh(`${SES}-newest`) + expect(idsOf(removed)).toEqual(["multi"]) + }) + + test("and it still keeps the block when even the newest tombstone predates the edit", async () => { + // The mirror image: picking the newest must not become "reap if any + // tombstone is old enough". + const removed = seeThrough({ multi2: { updated: "2026-08-01T00:00:00.000Z" } }) + listResponse = [ + archived("multi2", { archived_at: "2026-06-01T00:00:00.000Z" }), + archived("multi2", { archived_at: "2026-07-01T00:00:00.000Z" }), + ] + await refresh(`${SES}-newest-kept`) + expect(idsOf(removed)).toEqual([]) + }) + + test("a live record for ANOTHER project does not protect this project's block", async () => { + // The live set is built from the same identity rule the tombstones are, so + // the workspace/project tests have to apply to it too. A live record two + // projects over shares the block id and nothing else. + const removed = seeThrough({ scoped: { updated: "2026-05-01T00:00:00.000Z", scope: "project" } }) + listResponse = [ + { + id: "mem-live-elsewhere", + memory: "other project", + metadata: { + source: MIRROR_SOURCE, + block_id: "scoped", + block_scope: "project", + datamate_id: "42", + repo_remote: "https://github.com/acme/somewhere-else", + }, + }, + archived("scoped", { block_scope: "project", datamate_id: "42", repo_remote: BINDING.repoRemote }), + ] + await refresh(`${SES}-live-elsewhere`) + expect(idsOf(removed)).toEqual(["scoped"]) + }) }) describe("hydrate", () => {