fix(replication): per-key follower cache invalidation after apply — a connected lobe no longer serves deleted memories (#869) - #878
Open
likesjx wants to merge 1 commit into
Conversation
…ected lobe no longer serves deleted memories (scrypster#869) The applier holds a bare *pebble.DB and commits replicated writes straight to disk, underneath the storage layer's in-memory caches. A follower that had recalled an engram held it in the L1 cache; a replicated soft-delete then landed in Pebble beneath that entry, and every later recall kept serving the cached copy as active until the process restarted. Not just soft-deletes: evolve supersession, not_true_since invalidation, trust changes, tag updates and restore all ride the same path, and since every cache hit refreshes recency, the hottest memories were exactly the ones that never healed. The fix is the callback shape suggested in the issue triage: the applier takes an invalidation callback (SetInvalidate — a setter because server.go builds the applier with the coordinator before the storage layer exists), and after each commit feeds every user key the entry touched back through it. The OpBatch path decodes the applied repr with pebble.ReadBatch; single-key ops pass entry.Key. Invalidation runs strictly AFTER Commit — invalidating first would let a racing read re-cache the old Pebble state just before the commit lands, resurrecting the staleness this removes. The storage half maps applied keys to cache entries: 0x01 engram keys drop both the L1 entry (vault-scoped) and the metaCache entry; 0x02 meta keys drop the metaCache entry. Both caches, per the triage traps — missing metaCache leaves GetMetadata stale. Every other prefix passes through untouched: a follower applies constant Hebbian/decay traffic, and clearing more than the touched keys would make the caches useless (trap 2). assocCache/revAssocCache carry their own 2s TTL and are out of scope here (scrypster#818 tracks their gaps). Wiring lives in cmd/muninn/server.go next to storeCfg.RepLogAppend — the two hooks are mirror images (storage→replication for local writes, replication→storage for applied keys) and exist because neither package may import the other. Tests: - TestReplicatedSoftDelete_InvalidatesFollowerCache (external test package — the seam crosses an import cycle, mirroring why production joins the halves in server.go): primary ships real RepLogAppend batches, follower applies them, cache warmed by a read, delete applied, recall must observe it with no restart. RED-checked: with the two invalidation calls reverted it fails at exactly the stale read ("state = active, want StateSoftDeleted"). - TestInvalidateReplicatedKey_IgnoresForeignKeys: nil/short/long/wrong-prefix keys neither panic nor evict unrelated entries. go test ./internal/replication/ ./internal/storage/ green; go vet and gofmt clean.
There was a problem hiding this comment.
Pull request overview
This PR fixes replication correctness on follower/observer nodes by invalidating in-memory storage caches on a per-key basis after replication entries are applied to Pebble, ensuring replicated mutations (notably soft-deletes) are immediately visible without requiring a process restart.
Changes:
- Add a per-key invalidation callback to the replication applier and invoke it after commits for both batch and single-key replication ops.
- Implement
PebbleStore.InvalidateReplicatedKeyto evict only the affected engram/L1 and metadata cache entries based on replicated user keys. - Add regression and edge-case tests verifying replicated soft-deletes penetrate a warmed follower cache and that foreign/invalid keys are safely ignored.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/storage/replica_invalidation.go | Adds storage-side per-key cache eviction for replicated engram/meta keys. |
| internal/storage/replica_invalidation_test.go | Tests that invalidation ignores foreign/malformed keys and doesn’t evict unrelated entries. |
| internal/replication/invalidation_869_test.go | End-to-end regression test for replicated soft-delete invalidating a warmed follower cache. |
| internal/replication/applier.go | Adds invalidation callback plumbing and calls invalidation after apply/commit. |
| cmd/muninn/server.go | Wires the applier invalidation callback to the store during server construction. |
Suppressed comments (1)
internal/replication/applier.go:154
- The OpBatch invalidation runs only after the marker batch commits. If
reprBatch.Commit()succeeds butmarkerBatch.Commit()fails, the follower’s on-disk state has changed but caches will not be invalidated (and Apply returns an error, potentially stopping replication while caches keep serving stale data).
Move invalidateBatchKeys to immediately after reprBatch.Commit() so cache correctness tracks the actual data commit, not the bookkeeping marker.
// #869: the repr is committed — tell the storage layer which keys
// changed so its caches drop any entries the batch just mutated.
a.invalidateBatchKeys(entry.Value, entry.Seq)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+59
to
+64
| func (a *Applier) invalidateBatchKeys(repr []byte, seq uint64) { | ||
| if a.invalidate == nil { | ||
| return | ||
| } | ||
| r, _ := pebble.ReadBatch(repr) | ||
| for { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #869. Taking you up on "it's yours" — implemented to the scope in your triage comment, traps included.
The fix, in your frame
The applier gains an invalidation callback and feeds every user key an applied entry touched through it, after Commit. The OpBatch path decodes the applied repr with
pebble.ReadBatch; single-key ops passentry.Key. The storage half maps keys back to cache entries and drops exactly those.On the after-Commit ordering (not in the triage, but load-bearing): invalidating before the commit would let a racing read re-load the old value from Pebble and re-cache it just before the commit lands — resurrecting exactly the staleness this removes. After Commit, a racing read caches the new state, which is fine.
The four traps
Applier.SetInvalidate(func(key []byte)), wired incmd/muninn/server.goright next tostoreCfg.RepLogAppend; the comment there names them as mirror images. It's a setter rather than aNewApplierparameter only because of construction order: server.go builds the applier with the coordinator before the store exists.assocCache/revAssocCachehave their own 2s TTL story and stay out of scope (storage: three assoc-mutation sites evict neither association cache (COG-31 blind spot, up to 2s) #818).0x01engram keys drop the vault-scoped L1 entry and the metaCache entry;0x02meta keys drop the metaCache entry. metaCache is keyed by ID alone (ULIDs are globally unique), so the vault prefix only gates the L1 delete.TestReplicatedSoftDelete_InvalidatesFollowerCachefollows your spec: primary store ships realRepLogAppendbatches, a follower applier applies them, a read warms the follower's cache, the replicated soft-delete is applied, and recall must observe it with no restart. It lives ininternal/replicationas an external test package — the seam crosses thereplication ← cognitive ← storageimport cycle, which is also exactly why production joins the two halves in server.go rather than by import.A second test pins the invalidator's edges: nil/truncated/oversized/wrong-prefix keys neither panic nor evict unrelated entries — the applier feeds it every key of every batch, so it must be indifferent to shapes it doesn't own.
One behavior note
A decode failure in the applied repr is logged and abandoned rather than returned: the batch itself committed successfully, and failing the Apply would make the follower re-request an entry it already holds. The log line says which seq, and that caches may serve stale entries for that batch.
Verification
go test ./internal/replication/ ./internal/storage/— green (41.9s / 57.6s)go vet,gofmt— cleanNext: deploying this build to one of my macOS observers and running the #869 repro live against it (write → recall-on-lobe to warm → forget on Cortex → recall-on-lobe again, no restart). I'll report the result on the issue either way.