Skip to content

feat: atomic supersede via replaces on memory create#14

Merged
yhyyz merged 1 commit into
ourmem:mainfrom
doctatortot:upstream-clean/memory-supersede
May 21, 2026
Merged

feat: atomic supersede via replaces on memory create#14
yhyyz merged 1 commit into
ourmem:mainfrom
doctatortot:upstream-clean/memory-supersede

Conversation

@doctatortot
Copy link
Copy Markdown
Contributor

Closes #8.

Implements the design from #8 per your review there — replaces: string[] on POST /v1/memories for atomic consolidate-into-one. New memory is created, each listed old gets state=superseded, superseded_by=<new.id>, invalidated_at=now(). Default search/list now filters state NOT IN ('deleted', 'superseded'); opt-in via include_superseded=true.

Maintainer points addressed

Point from #8 review This PR
A1 (replaces on POST), not A2
Reuse invalidated_at, no new timestamp supersede_batch sets it
New Superseded state variant MemoryState::Superseded + FromStr/Display + filter integration
Default search/list excludes superseded state NOT IN ('deleted', 'superseded') everywhere; include_superseded=true on /search and /memories opts back in
memory_get returns superseded regardless get_by_id already unfiltered, unchanged
400 with list of missing IDs supersede_batch precheck collects missing + already-superseded IDs and returns Validation with both lists; handler maps to 400
No cross-tenant 403 (physical isolation) ✅ unknown ID → missing
Tag inheritance: none ✅ caller controls tags on new memory
Plugin: replaces on memory_store

One open question: you suggested 409 on already-superseded; I implemented it as 400 Validation with explicit already superseded: [...] in the error body, so it shares the precheck shape with missing-IDs. Happy to switch to a distinct 409 if you'd rather. Let me know in review.

Atomicity caveat

LanceDB has no multi-row transactions, so supersede_batch follows the same shape as your existing handle_supersede in reconciler.rs:231:

  1. Precheck every old_id exists + isn't already superseded. On any failure, return Err before any write happens.
  2. Create the new memory.
  3. Mark each old as superseded (sequential update calls).

If step 3 partially fails after step 2 succeeded, the new memory exists but the chain is partial. The error names the failing IDs so the caller can retry the marks. Worth documenting; alternative would be a soft-rollback that deletes the new on partial failure — I leaned toward the same shape as the existing internal precedent.

Tests

Store layer (4 new):

  • test_supersede_batch_marks_old_as_superseded
  • test_supersede_batch_rejects_missing_id (precheck → no writes)
  • test_supersede_batch_rejects_already_superseded
  • test_default_state_filter_excludes_superseded (+ include_superseded=true resurfaces, + get_by_id still returns)

API layer (3 new):

  • test_create_with_replaces_supersedes_old
  • test_create_with_missing_replaces_returns_400 (asserts the real ID's state is still active — no half-state)
  • test_search_excludes_superseded_by_default (+ include_superseded=true opt-in)

Test totals

386 passed; 0 failed. Was 379 before this PR. +7 new, zero regressions.

🤖 Generated with Claude Code

Closes ourmem#8. Lets a caller consolidate fragmented memories into one new
memory in a single API call, marking the olds as superseded so search
naturally hides them while history stays recoverable.

## API surface

* `POST /v1/memories` accepts an optional `replaces: string[]`. When
  present, the new memory is created and each old `id` in the list is
  marked `state=superseded`, `superseded_by=<new.id>`,
  `invalidated_at=now()`.
* `GET /v1/memories/search` and `/v1/memories` (list) accept
  `include_superseded=true` to opt back into seeing them.
* `PUT /v1/memories/:id` already accepted `state` via ourmem#10; it now
  parses `state=superseded` since the variant exists.
* MCP plugin: `memory_store` gains an optional `replaces` array;
  description nudges the agent toward consolidate-not-delete semantics.

## Decisions matching the design comment

| Maintainer point | Implementation |
|---|---|
| A1 (`replaces` on POST), not A2 | Done. |
| Reuse `invalidated_at`, no new timestamp | `supersede_batch` sets it. |
| New `Superseded` state variant | Added (`MemoryState::Superseded`). |
| Default search/list excludes superseded | Filter is now `state NOT IN ('deleted', 'superseded')`; `include_superseded=true` opts back in. |
| `memory_get` returns superseded regardless | `get_by_id` was already unfiltered; unchanged. |
| 400 with list of missing IDs | `supersede_batch` precheck collects missing + already-superseded IDs and returns `Validation` with both lists; handler maps to 400. |
| No cross-tenant check | Per design, stores are physically isolated; unknown ID → missing. |
| 409 on already-superseded | Currently 400 (`Validation`) with explicit `already superseded: [...]`. Happy to switch to 409 if you'd rather have a distinct status — let me know. |
| Tag inheritance: none | Caller controls tags on the new memory. |
| Plugin: `replaces` on `memory_store` | Done. |

## Atomicity caveat

LanceDB has no multi-row transactions. `supersede_batch` does the
precheck up front (no writes happen on bad input) then creates the new
memory, then loops marking olds. If a per-old mark fails after the new
exists, the call returns `Storage(...)` naming the IDs that failed.
This matches the semantics of the existing `handle_supersede` in
`reconciler.rs:231` (create new → mark old).

## Tests

Store layer (4 new):
- `test_supersede_batch_marks_old_as_superseded`
- `test_supersede_batch_rejects_missing_id` (precheck, no writes)
- `test_supersede_batch_rejects_already_superseded`
- `test_default_state_filter_excludes_superseded`

API layer (3 new):
- `test_create_with_replaces_supersedes_old`
- `test_create_with_missing_replaces_returns_400`
- `test_search_excludes_superseded_by_default` (+ verifies
  `include_superseded=true` resurfaces them)

Full lib suite: **386 passed; 0 failed** (was 379 before this PR;
+7 new tests, zero regressions).

Co-Authored-By: Claude Opus 4.7 <[email protected]>
yhyyz pushed a commit that referenced this pull request May 20, 2026
`lance-encoding`'s build.rs invokes `protoc` to generate prost bindings.
Without it `cargo clippy` and `cargo test` fail at compile time with
"Could not find `protoc`. If `protoc` is installed, try setting the
`PROTOC` environment variable..."

This was masked previously because CI bailed at the `cargo fmt --check`
step (229 sites of fmt drift across upstream main). After the fmt
cleanup in 508e9e6, PRs now reach the clippy step and surface this dep.
Three currently-open PRs (#11 #12 #14) hit this on their last CI runs.

Adding `apt-get install protobuf-compiler` before the rust toolchain
step matches the standard pattern for Rust projects with prost-build
deps (see lancedb, datafusion, deltalake CI configs).

Co-Authored-By: Claude Opus 4.7 <[email protected]>
@yhyyz yhyyz merged commit 28ab2c3 into ourmem:main May 21, 2026
1 check failed
yhyyz added a commit that referenced this pull request May 21, 2026
PR #14 added include_superseded param to list(), PR #20's tests
used the old 2-arg signature. Updated to list(100, 0, false).
@yhyyz
Copy link
Copy Markdown
Contributor

yhyyz commented May 21, 2026

Merged and deployed! Thanks @doctatortot — excellent implementation that follows the design we agreed on in #8 to the letter. 386→388 tests, all green. 🚀

The replaces precheck (400 with missing/already-superseded lists) is a good design choice — using 400 Validation instead of 409 is fine, keeps it consistent with the existing error shape.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add memory supersede: atomic consolidate of multiple memories into one

2 participants