refactor(memory): split into history + recall, add retrieval pipeline + bench#33
Merged
Conversation
…etrieval, add locomo bench - sdk/memory: extract ltm into subpackage with extractor/merger/save split, add SaveWithContext, soft-merge, MD5 dedup, scope-aware operations - sdk/retrieval: unify retrieval pipeline (factory, stages, boost/decay, llm reranker, score threshold, entity extraction) - sdkx/retrieval: sqlite + postgres index backends with shared test suite - bench: promote to top-level standalone module via go.mod replace directives; add locomo benchmark harness (ingest/eval/judge) with qwen + azure provider support and per-question fault tolerance Made-with: Cursor
Workspace.Rename is the canonical "publish a finalized payload" primitive: - LocalWorkspace.Rename uses os.Rename (atomic on POSIX) - MemWorkspace.Rename moves entries in-map under the existing lock - ScopedWorkspace.Rename enforces write permission on both src and dst - objstore.Rename falls back to copy + delete (object stores lack atomic rename); doc clarifies the trade-off AtomicWrite wraps Write-tmp + Rename so concurrent readers never observe a half-written file. Replaces the old non-atomic free-function Rename helper, which had no callers. Made-with: Cursor
…lias
This is the first half of splitting sdk/memory into a clearly-named pair:
* sdk/history (this commit): conversation transcripts (buffer, lossless DAG,
archive, summary store, file store, sanitize, tools, counter).
* sdk/memory/ltm (unchanged here, moves to sdk/recall in a follow-up commit):
long-term fact extraction & retrieval.
What this commit changes is intentionally narrow:
1. git mv every non-ltm file from sdk/memory/ to sdk/history/ and rewrite
the package clause (package memory -> package history). No type, function,
or behavior changes.
2. Delete sdk/memory/ltm_alias.go. The pre-Phase-3 trampoline that re-exported
ltm.* under sdk/memory.* is gone; callers that used those aliases now
import sdk/memory/ltm directly. Inside sdk/history we qualify the same
ltm.* symbols (LongTermConfig, LongTermStore, MemoryAwareMemory, etc.).
3. Update the four call sites that imported sdk/memory:
- sdkx/llm/memory_integration_test.go (history.NewBufferMemory + ltm.*)
- sdkx/memory/jobqueue/sqlite/queue_test.go (ltm.* only)
- bench/locomo/{eval,runners,runners/flowcraft,cmd/ingest} (ltm.*; the
flowcraft runner also keeps history.* for its short-term store).
4. sdk/memory/ now contains only the ltm/ subdirectory. It will disappear
entirely once sdk/memory/ltm moves to sdk/recall (next commit).
Why now: the old sdk/memory tree mixed two packages (history vs LTM) plus
a backward-compat shim under one name, which made the public surface
ambiguous and made every later refactor need a "memory or memory/ltm?"
disambiguation. Splitting the rename out keeps the diff reviewable and lets
CI lock in a green baseline before any interface changes land.
make vet + make test pass on sdk, sdkx, examples/voice-pipeline. bench
passes with GOWORK=off go test ./... (bench is intentionally outside
go.work and uses replace directives).
Made-with: Cursor
Second half of the sdk/memory split. After this commit sdk/memory/ no
longer exists; long-term agent memory lives at sdk/recall.
Mechanics:
1. git mv every file from sdk/memory/ltm/ to sdk/recall/ (26 files,
all 100% similarity per git's rename detection).
2. Rewrite the package clause: package ltm -> package recall, including
the external test package (package ltm_test -> package recall_test in
memory_test.go).
3. Rewrite every importer: import path
"github.com/GizClaw/flowcraft/sdk/memory/ltm" -> ".../sdk/recall"
and qualifier ltm.X -> recall.X. Affected files:
- sdk/history/{memory,factory,memory_test,factory_test}.go
- sdkx/llm/memory_integration_test.go
- sdkx/memory/jobqueue/sqlite/{queue,queue_test}.go
- bench/locomo/{eval,runners/runner,runners/flowcraft/runner,
cmd/ingest/main}.go
4. Refresh sdk/recall/doc.go to describe the package by its new name
(the previous "Compared with the legacy sdk/memory.LongTermStore"
contrast no longer makes sense — that legacy is gone).
5. Remove the now-empty sdk/memory/ and sdk/memory/ltm/ directories.
What is intentionally NOT in this commit:
- No interface, type, or behavior changes. recall.Memory, recall.Entry,
recall.NewRetrievalStore, recall.MemoryAwareMemory, etc. all keep the
exact same signatures they had as ltm.*. The Save->Append rework, the
Config -> functional-options migration, and the removal of the
Assembler / MemoryAware glue are scheduled for separate commits.
- Telemetry meter and span names ("memory.dag.ingest", "memory.archive",
...) are kept as-is to avoid breaking dashboards. They will be
revisited together with the interface refactor.
- sdkx/memory/jobqueue/sqlite stays in sdkx/memory/ for now; its move
to sdkx/recall/jobqueue/sqlite is a later commit so this one stays a
pure rename.
make vet + make test pass on sdk, sdkx, examples/voice-pipeline, and
bench (GOWORK=off go test ./...).
Made-with: Cursor
…ability bugs
Replace the lossy Memory.Save(fullHistory) contract with Memory.Append(newOnly)
and close several persistence-correctness holes in the lossless backend.
Interface change
----------------
The previous contract required callers to pass the *entire* conversation
history on every write; the implementation diffed it against GetMessages
to discover what was new. That contract had two structural problems:
1. Read-modify-write race. Two goroutines that called Save on the same
conversation could both observe the pre-state, both diff against it,
and both overwrite — silently dropping one side's writes. There was
no per-conversation lock.
2. Truncated histories were silently accepted (and silently shrank the
persisted log) because Save trusted the caller's slice as the new
ground truth.
The new shape is
type Memory interface {
Load(ctx, convID) ([]Message, error)
Append(ctx, convID, newMessages []Message) error
Clear(ctx, convID) error
}
There is no compat shim for Save: only one external caller existed
(recall.MemoryAwareMemory), the four in-tree tests, and one bench runner.
Keeping a deprecated Save would have meant keeping the racy code path
alive too — defeating the point of this commit.
P0 durability fixes inside sdk/history
--------------------------------------
* lossless.go: per-conversation lock around Append. The semaphore-bounded
background ingest is gone; it could silently drop DAG ingests under load
(telemetry warned, but the summarized history quietly shrank). Each
Append now schedules its own goroutine and is naturally serialized for
the same conversationID by the lock; cross-conversation parallelism is
unbounded as before but no longer fights for a fixed-size pool.
* lossless.go: ingest and archive get independent timeouts (defaultIngestTimeout,
defaultArchiveTimeout) instead of sharing a single 60s budget. A slow
LLM summarization can no longer starve archive (or vice-versa).
* archive.go: SaveManifest, writeIntent now go through workspace.AtomicWrite
(write-tmp + rename), so a crash mid-write cannot leave RecoverArchive
staring at a half-serialized manifest or intent file. deleteIntent uses
ws.Delete instead of overwriting with an empty payload (which left a
zombie file that made Exists() lie).
* summary_store.go: Rewrite uses workspace.AtomicWrite. A crash during
the rewrite previously risked a half-truncated summaries.jsonl that
the line scanner would silently treat as missing summaries.
* summary_store.go: FileSummaryStore caches are bounded (LRU, default
cap 1024 conversations). The previous unbounded map leaked memory
proportional to the conversation cardinality the process had ever
touched. Eviction drops the cache slice and the per-conversation lock
together (a stale lock with no cache is just a leak; reconstructing
it on next access is free). New WithSummaryStoreCapacity option for
callers that need a different bound. As a side effect, appendCache
no longer materializes a "cache of one" when the entry has not been
loaded — that prior behavior could fool subsequent List/Search into
thinking the conversation only had the just-appended node.
* buffer.go: Append takes a per-conversation lock too. When the Store
satisfies MessageAppender, we delegate; otherwise we do the get +
concat + save fallback under the lock so it stays race-free.
Callers and tests
-----------------
* sdk/recall/aware.go: ShortTermMemory interface now declares Append (it
is the local mirror of history.Memory). MemoryAwareMemory.Append simply
forwards.
* sdk/recall/aware_test.go: in-package fake bufferMemory implements Append
with the same get+concat+save fallback.
* sdk/history/{buffer,file_store,lossless,compact}_test.go: call sites
switch from Save(fullHistory) to Append(newOnly). The two tests that
intentionally re-passed the loaded prefix have been updated to pass
only the freshly produced messages — they were testing the racy
contract, not real user behavior.
* sdk/history/lossless_test.go: TestLosslessMemory_SemaphoreFull is
replaced by TestLosslessMemory_NoIngestDrop, which pins the new
invariant: 20 rapid Appends across 20 conversations all complete with
no silent drops. The previous test only confirmed the (now removed)
drop-on-full path.
* sdk/history/summary_store_test.go: cache white-box assertions go
through a new cacheSnapshot test helper that reads the LRU entry by
conversation id (instead of poking the removed `cache` map directly).
Verification
------------
make vet and make test pass on sdk, sdkx, examples/voice-pipeline.
go test -race ./history/ ./recall/ is clean. bench passes with
GOWORK=off go test ./...
Made-with: Cursor
…, drop aware/assembler glue
The recall API surface had three independent shapes for the same idea
("which bucket does this read/write live in"): persistence Scope on
Entry, query Scope on List/Search, and a separate RecallScope for
multi-partition union. They were also leaking through a stateful
MemoryAware/ContextAssembler glue that mutated runtimeID/scope on the
instance — causing user IDs to leak across requests in any reused
wrapper. This commit collapses the surface to one type and removes the
glue layer:
Constructor
- recall.New(idx, ...Option) replaces recall.New(Config). Options are
self-documenting and additive: WithLLM/WithEmbedder/WithJournal/
WithSaveContext/WithoutMD5Dedup/WithoutSoftMerge/etc. The previous
"all-zero booleans auto-flip to ON" heuristic in Config was removed;
defaults are now declared in one place inside New.
Scope (one type, not three)
- MemoryScope -> Scope; MemoryEntry -> Entry; MemorySource -> Source;
MemoryCategory -> Category; MemoryPartition -> Partition;
LongTermStore -> Store. The package qualifier already says "recall",
the Memory* prefix was redundant.
- RecallScope is gone; its Partitions field moved onto Scope, with a
new Scope.EffectivePartitions() that auto-derives [PartitionUser] or
[PartitionGlobal] from UserID when the caller does not set it
explicitly. List/Search read partitions exclusively from
opts.Scope.Partitions, so callers no longer pick between two ways
of saying the same thing.
- Scope.SessionID is removed. It conflated conversation-thread state
(which belongs in sdk/history) with long-term-fact partitioning,
leaked thread IDs into the namespace and the deterministic ID hash,
and forced every recall caller to thread a session ID even when
they only wanted user-level facts. Callers that previously
partitioned by session should either keep that data in
sdk/history (transient) or model it as Entry.Keywords (durable).
Note this is a breaking schema change: deterministicEntryID now
omits the session line, so re-ingest may produce duplicates of
facts that were extracted before the upgrade.
Glue layer removed
- Deleted aware.go (MemoryAwareMemory), assembler.go, assembler_cache,
assembler_metrics, and their tests (~1750 LoC). They wrapped
history.Memory + recall.Store and prepended a "[Long-term memory]"
block to the system prompt; they also held mutable runtimeID/scope
on the instance, which leaked per-user state across reuse and made
SetScope a footgun. The replacement is a ~10-line snippet at each
call site: recall.Memory.Recall() -> format hits -> prepend to the
system prompt before calling LLM. examples/chatbot-with-recall (in
a follow-up commit) shows the canonical shape.
- sdk/history.Config.LongTerm is removed and history.NewWithLLM no
longer takes an ltStore parameter; the history package no longer
imports recall. This removes a circular layering pressure between
"transcript storage" and "fact recall".
Call-site updates
- bench/locomo runners and the SQLite jobqueue tests switch to
recall.New(idx, ...Option).
- sdkx/llm/memory_integration_test.go drops the MemoryAware path and
builds the with-memory variant directly: Recall() -> format ->
prepend to system prompt -> Generate. This is the same pattern
examples/chatbot-with-recall will demonstrate.
Verified with make ci (all sdk/sdkx tests, plus go test -race on
sdk/recall and sdk/history) and a manual `go test ./...` from bench/.
Made-with: Cursor
… example Finishes the sdk/memory → sdk/history + sdk/recall split by removing the last sdkx/memory/* path and shipping a runnable composition demo. * git mv sdkx/memory/jobqueue/sqlite → sdkx/recall/jobqueue/sqlite Keeps file history; the wrapper directory under sdkx/memory/ is now fully removed. Updates the lone comment reference in sdk/recall/jobs.go. * Renames the persistent table from memory_jobs to recall_jobs to keep the schema names aligned with the package name. This is a breaking on-disk change — doc.go now spells out the one-liner ALTER TABLE needed to migrate an existing queue. * Adds examples/chatbot-with-recall, a ~150-line self-contained demo showing the recommended history+recall composition that replaces the deleted MemoryAwareMemory wrapper. Uses in-memory stub LLMs so it runs network-free; README explains how to swap in a real provider and a SQLite job queue. Intentionally not added to go.work — run with GOWORK=off go run . from the example directory. Verified: make ci (sdk/sdkx green) + GOWORK=off go test ./... in bench + GOWORK=off go run . in examples/chatbot-with-recall. Made-with: Cursor
… RFC Closes the gap between the post-split implementation and the design note in docs/memory-refactor.md so callers see one consistent shape. sdk/recall: - Slim Memory to the read/write core (Save/SaveAsync/Add/Recall/ Forget/Close); pull History+Rollback into Auditable and JobStatus+ AwaitJob into JobController. The canonical lt impl satisfies all three so tests just type-assert. - Rename RecallRequest→Request, RecallHit→Hit, AddRaw→Add. - Drop Store / RetrievalStore / ListOptions / SearchOptions / Embedder / VectorSearcher / NormalizeScopeForCategory / DefaultGlobalCategories. Memory.Recall is the only public read path; callers needing raw enumeration drop to retrieval.Index directly (see the rewritten sdkx/llm/memory_integration_test.go for the pattern). - Recall search tests now exercise Memory.Recall instead of the removed RetrievalStore. sdk/history: - Memory.Load(ctx, convID) → Memory.Load(ctx, convID, Budget). Zero Budget keeps the old "use defaults" semantics; non-zero MaxTokens / MaxMessages clamp. - Replace exported BufferMemory / LosslessMemory + their constructors with NewBuffer(store, BufferOption…) and NewCompacted(store, llm, ws, CompactOption…) returning unexported buffer / compacted types. - Delete history.Config and history.LosslessConfig; per-knob CompactOption replaces them. NewWithLLM is gone. - Rename history_expand / history_compact tools (was memory_*) and bump the LLM-node schema hint to match. examples/chatbot-with-recall: - Split into main.go (≤80 lines, the wiring) + stubs.go (offline LLM doubles + demoTurns + buildPrompt/formatRecallBlock) so the example is readable at a glance. - Pick up history.NewBuffer / history.Budget / recall.Request / Hit / Add naming. bench/locomo, sdkx/llm integration tests, sdkx/recall/jobqueue/sqlite: - Updated for the renames and the JobController / Auditable type-assertion pattern. Verified: make ci, GOWORK=off go test ./... in bench, GOWORK=off go run . in examples/chatbot-with-recall, go vet -tags=integration sdkx/... Made-with: Cursor
Strict alignment with §4.1 of docs/memory-refactor.md (Scheme A):
- Rename interface Memory→History (matches package name; eliminates
collision with recall.Memory)
- File renames:
- memory.go → history.go (interface + Budget only)
- lossless.go → compactor.go (also absorbs summary_dag.go,
summary_index.go, factory.go's NewCompacted/Options — all
summarization concerns now live in one file)
- file_store.go + InMemoryStore → store.go (one file for the
persistence interface and shipped impls)
- counter.go → token.go (matches RFC label "TokenCounter")
- summary_prompt.go → compactor_prompt.go
- Internal compacted struct → compactor (matches file name)
- Add doc.go (package overview the RFC explicitly calls out)
- Drop factory.go entirely; NewBuffer lives next to buffer in
buffer.go, NewCompacted next to compactor in compactor.go
- Test files renamed to mirror sources
Public API change: history.Memory → history.History. Single caller
(examples/chatbot-with-recall) updated. No production deployments
exist yet; no compat shim added.
verified: make ci + GOWORK=off bench + example demo all green.
Made-with: Cursor
Made-with: Cursor
Two new bench surfaces filling gaps surfaced during the memory refactor:
1. bench/history-compression/
Long-context QA quality vs prompt-token cost across three strategies
(none / buffer / compacted). Reuses bench/locomo's dataset + metrics
so the two reports are directly comparable. CLI mirrors locomo's
provider:model flag plumbing. Compacted strategy auto-skips when
--summary-llm is empty so CI smoke stays LLM-free.
Runs against synthetic in <1s with a stub LLM; pointed at LoCoMo10
it produces (qa_judge, prompt_tokens_p95, load_latency_p95) per
strategy — i.e. the "is the compactor's compression free?" question
becomes a single diff.
2. sdkx/recall/jobqueue/sqlite/bench_test.go
Three Go micro-benchmarks measuring the SaveAsync hot path:
- Enqueue (memory + file modes)
- Lease+Complete drain loop
- SaveAsync end-to-end at workers=1/4/16 with a fake LLM, which
isolates queue + persistence overhead from extractor latency.
Sample numbers on M2 Max:
Enqueue/memory ~74K ops/s
Enqueue/file ~26K ops/s (fsync-bound)
Lease+Complete ~13K ops/s
SaveAsync e2e ~900 ops/s (single-writer SQLite is the floor)
These numbers establish the baseline for future jobqueue work
(batched commits, alternative backends).
CI: make ci green; bench/history-compression unit test green under
GOWORK=off; sdkx/recall/jobqueue/sqlite already in make ci.
Made-with: Cursor
Adds the truncation signal that was previously stubbed at 0. The detector fingerprints each evidence-bearing turn at ingest (first ~32 normalized runes — long enough to be conversation-unique, short enough to survive compactor rephrasing of trailing words) and after every Load checks that at least one fingerprint still appears in the loaded prompt. Three new report fields: - evidence_measured: questions that carried any evidence_id - truncated: of those, how many lost ALL evidence turns - truncated_rate: truncated / evidence_measured Operationally this lets reports separate two failure modes that previously collapsed into qa_judge ↓: 1. compactor compressed the evidence away → truncated > 0 2. model failed to use evidence still in prompt → truncated = 0 Tests: - TestRun_TruncatedDetected: BufferMax=2 must report Truncated=1 for a 4-turn conv whose evidence is in turn 1 - TestRun_NoEvidenceLeavesTruncatedZero: datasets without evidence_ids must report 0/0/0 (no false positives, no div-by-zero) CLI prints `truncated=k/N (rate%)` for evidence-bearing runs, `n/a` otherwise. Made-with: Cursor
… log A 1542-question full LoCoMo run was strictly sequential, so even with qwen-turbo (~3s/call) one strategy took ~25min and gpt-5.4 was unrunnable. Per-conversation Load is concurrent-safe (compactor and buffer both lock per conv, ingest happens before the loop), so the answer/judge fan-out can shard across goroutines without changing the result. Adds Options.Concurrency (default 1, opt-in) and Options.ProgressEvery, plus matching --concurrency / --progress-every CLI flags. With --concurrency 8 the full LoCoMo10 run drops from "many hours" to ~16min (none 7min, buffer 4min, compacted 5min) on qwen-turbo. Made-with: Cursor
… b.N Default -benchtime=1s lets the harness inflate b.N to ~6500 jobs, but workers=1 drains them serially and a fixed 30s per-await timeout expired on ~15% of jobs, failing the bench. Scale the timeout proportionally to b.N/workers (1ms per queued predecessor, floored at 30s, no upper cap) so single-worker runs complete on slow CI hardware without spurious failures. Made-with: Cursor
Mirrors locomo/results/: per-run report JSON is reproducible from the dataset + flags and bulky enough to clutter diffs. Made-with: Cursor
Three correctness-leaning fixes surfaced during the post-refactor review:
1. recall.Add: validate Content before deriving any state.
Previously the empty-content error was returned AFTER the deterministic
ID was hashed, the timestamps stamped, and the scope copied — so a
throwaway ltm_<sha256> ID could land in debug logs / span attrs for
an entry that was about to be rejected. Move the check to the top of
the method.
2. history.Closer: expose compactor.Close to History consumers.
history.NewCompacted returns a History whose async ingest/archive
goroutines outlive each Append, but the History interface had no Close
method, leaving callers no way to drain on shutdown — the goroutines
leaked until process exit. NewBuffer is stateless and intentionally
does NOT satisfy Closer, so the canonical pattern is a type-assert:
hist := history.NewCompacted(store, llm, ws)
defer func() {
if c, ok := hist.(history.Closer); ok { c.Close() }
}()
compile-time assert added on *compactor; chatbot example updated to
demonstrate the type-assert pattern even though it uses NewBuffer.
3. recall: bound Memory.Close() with a per-job context.
The async worker previously ran Extract / upsertFacts under
context.Background(), so a stuck LLM call would pin Memory.Close()
on m.wgWorkers.Wait() forever — wg.Wait() had no upper bound at all.
New three-tier context model:
- workerCtx (cancelled by Close): root for Lease + jobCtx, lets Close
unblock blocking Lease implementations promptly.
- jobCtx = WithTimeout(workerCtx, jobTimeout): wraps Extract +
upsertFacts so a single misbehaving job is bounded by the configured
timeout (default 5min, override via new WithJobTimeout option).
- bookCtx = WithTimeout(Background, 5s): wraps Complete / Fail /
Reschedule. Derived from Background, NOT workerCtx, so Close()'s
cancel of workerCtx cannot orphan a successful job in 'running'
state.
Close() is now idempotent (guarded select on stopCh + cancel is a
no-op the second time) and is bounded by jobTimeout in the worst
case (extractor that ignores ctx.Done()).
Tests
-----
- TestAddEmptyContentFailsFast pins the validation order.
- TestCloseCancelsStuckJob uses a blockingLLM that only returns on
ctx.Done() and asserts Close() returns within 5s (vs the 2-minute
jobTimeout configured on the test instance) — proving Close cancels
in-flight jobs rather than waiting them out.
make ci green; go test -race ./recall/... clean.
Made-with: Cursor
Two fixes to the per-module loop:
1. Errors no longer get swallowed.
The previous form
@for m in $(MODULES); do echo "==> $$m"; ( cd $$m && go vet ./... ); done
ran each submodule in a subshell, but the shell-level for-loop's exit
status is the status of `done`, not of the failing subshell. A `go vet`
failure in any module printed the error but make recorded exit 0 — so
"make ci green" was, on this branch, occasionally a lie. Bench in
particular has been silently failing vet ever since it was promoted to
a separate module (it requires GOWORK=off, which the loop did not
set).
The new GO_FOREACH macro prefixes `set -e` and re-tests after the
subshell, so the first module that fails terminates the whole target
with the right exit code. Verified by injecting a deliberate vet error
in sdk/recall — `make vet` now exits 1 with the offending file in the
output.
2. bench is back in CI for vet + test.
bench/ lives outside go.work (heavyweight eval datasets/CLIs that
would dirty the workspace dependency graph) and needs GOWORK=off. The
new MODULES_OFFWORK list captures that, so the loop transparently
prefixes GOWORK=off for those modules.
Cost check: bench's actual *test* surface (history-compression +
locomo evals running against in-memory stubs) is ~1.6s on M2 Max;
total `make ci` is ~40s end-to-end. The expensive paths
(bench/locomo/cmd/eval, history-compression/cmd/eval) are main
packages — `go test ./...` does not invoke them, so dataset / LLM
calls stay opt-in.
3. Help text rewritten to spell out the bench-in-CI policy (vet + test
yes, eval CLIs no).
Made-with: Cursor
…ationale Three godoc blocks pointed at files that do not (and likely will not) exist in this repo: - sdk/recall/doc.go -> docs/memory-refactor.md - sdk/recall/merger.go -> RFC-0002 - sdk/retrieval/pipeline/stages_boost.go -> RFC-0002 `go doc` rendered them as broken jump targets, and grep'ing the repo for the referenced filenames returned only the references themselves — classic "doc that promises a doc". Rather than backfill the missing files, inline the actual content the comments needed: * sdk/recall: replace the migration-doc pointer with self-contained v0.2.0 migration notes — covers the ltm.X -> recall.X rename, the Scope.SessionID removal (and its impact on deterministicEntryID, i.e. treat the LTM index as a fresh corpus), and the explicit instruction to compose history.Memory + recall.Memory at the call site instead of reaching for the deleted MemoryAware glue. * recall.merger / retrieval boost stage: replace the RFC pointer with the soft-merge contract itself — supersession is reversible (we keep the old entry + a SupersededBy edge rather than deleting), and SupersededDecay applies a multiplicative penalty so the predecessor drops below the survivor in normal ranking but resurfaces on direct ID lookup. No behavioural change; only comments touched. Goal is that future readers can answer "why does this code exist" entirely from the file in front of them. Made-with: Cursor
Post-merge alignment: main bumped sdkx → sdk v0.1.12 in #32. bench lives outside go.work and uses a `replace ../sdk` for builds, but `go vet` / `go mod tidy` still verify the version pin matches what the workspace's other modules declare. After merging origin/main, `make vet` complained: go: updates to go.mod needed; to update it: go mod tidy Bump bench's pin to v0.1.12 to match. No transitive deps changed (go.sum is untouched) because the actual build still resolves through the local replace. Made-with: Cursor
The smoke test boots a compactor against t.TempDir(), Appends one
message, and returns. Background ingest/archive goroutines were still
flushing to disk when t.Cleanup tried to RemoveAll the tempdir,
producing flaky failures of the form:
TempDir RemoveAll cleanup: unlinkat .../TestNewCompacted_SmokeBoots...:
directory not empty
This is exactly the leak the new history.Closer interface exists to
fix. Use it:
t.Cleanup(func() { if c, ok := mem.(Closer); ok { c.Close() } })
Reproduced once on this machine after merging main; -count=5 green
after the fix.
Made-with: Cursor
Aligns the new long-term-memory surface (sdk/recall) and the shared
retrieval pipeline with the OTel coverage already in sdk/history
(memory.archive, memory.dag.*). Without these signals the hottest
paths in the refactored memory layer — Add, Recall, the async Save
worker, and every retrieval stage — were a black box in production:
no spans for latency attribution, no counters for error rates, no
warn-level log when an extractor wedged past jobTimeout.
What gets emitted
-----------------
sdk/recall/telemetry.go (new) centralises the meter so suffixes stay
canonical (memory.recall.*, paralleling memory.archive / memory.dag).
Histograms are in seconds; counters carry only enum-style labels
(outcome / stage / reason). Scope.UserID is deliberately never an
attribute — it is PII and would explode metric cardinality.
Spans:
memory.recall.add (write path; child upserts inherit)
memory.recall.recall (read path; pipeline stages become children)
retrieval.stage.<name> (one per pipeline stage, parented by recall)
Counters:
memory.recall.add_total{outcome=success|fail, reason=...}
memory.recall.recall_total{outcome=success|fail, reason=...}
memory.recall.job_total{outcome=succeeded|failed|timeout|dead, stage, attempts}
memory.recall.job_lease_errors_total
Histograms:
memory.recall.add_duration (seconds)
memory.recall.recall_duration (seconds)
memory.recall.recall_hits (count, per call)
memory.recall.job_duration (seconds, extract + upsert)
Logs (telemetry.Warn, trace-correlated):
"recall: worker lease failed"
"recall: async job failed" (with outcome=timeout|failed)
"recall: async job dead-lettered"
"recall: job complete bookkeeping failed"
Notes
-----
- handleJob splits "extractor returned err" from "ctx.DeadlineExceeded"
via errors.Is, so dashboards can isolate "extractor wedged past
jobTimeout" from "extractor returned business error". This pairs with
the per-job timeout added earlier in this branch.
- failOrRetry distinguishes dead-letter (Fail) from transient retry so
alert thresholds can target the dead path, not the retry noise.
- pipeline.Run keeps the pre-existing StageTrace bookkeeping; the new
spans are additive and let downstream OTel collectors do the same
attribution without parsing State.Trace.
- Query text and Scope.UserID are intentionally absent from span
attributes (PII + cardinality). Only structural shape is captured
(query_len, top_k, has_user_id, hits).
Verified
--------
go vet ./recall/... ./retrieval/... # clean
go test ./recall/... ./retrieval/... -race -count=1 # all green
make ci # full pipeline green, ~40s
Made-with: Cursor
Collaborator
Author
Suggested review orderIf you only have 30 minutes:
Things to ignore on first pass
Top three things I want eyes on
|
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
1 participant
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.
TL;DR
sdk/memoryis split into two orthogonal packages with distinct lifetimesand concerns:
sdk/history— short-term conversation transcripts (Buffer / Compacted)sdk/recall— long-term extracted facts with retrieval pipeline,async Save jobs, journal, and soft-merge
The cross-domain glue (
MemoryAware,ContextAssembler, implicit LTinjection in
Load) is deleted in favor of explicit composition atthe call site (~10-line pattern, see
examples/chatbot-with-recall/).A unified retrieval layer (
sdk/retrieval+sdk/retrieval/pipeline) isintroduced and used by
recall. Knowledge can adopt it incrementallyin a follow-up — no API change required for that to happen.
A
bench/module is added for offline evals (bench/history-compression,bench/locomo) and is wired intomake ci(vet + unit tests only;heavyweight eval CLIs are
mainpackages and stay opt-in).Why
Three structural problems on the old
sdk/memory:Memorydid three things at once — conversation window,long-term fact store, and automatic prompt assembly. Callers couldn't
inspect what the LT layer pulled, couldn't dry-run, couldn't swap
composition order.
Save(fullHistory)had a read-modify-write racethat silently dropped messages under concurrent writers; compactor's
global semaphore silently dropped DAG ingests under load. Both fixed
here with per-conversation locks + atomic writes
(
workspace.AtomicWrite).in some fields and "zero-value-means-off" in others, and adding new
knobs broke source compat. Replaced with functional options across
recall.New,history.NewBuffer,history.NewCompacted.Highlights
History (
sdk/history)New file layout aligned with the memory-refactor RFC.
Memorycontract tightened toAppend(newOnly); per-conversation locksprotect against concurrent writers.
File-backed stores use
workspace.AtomicWrite(write-tmp + rename) forcrash recovery.
New
history.Closersub-interface so callers of `NewCompacted` candrain background ingest/archive goroutines on shutdown.
Type-assert pattern (NewBuffer is intentionally not Closeable):
```go
defer func() { if c, ok := hist.(history.Closer); ok { c.Close() } }()
```
Recall (
sdk/recall)sdk/memory/ltm; no symbol-level renames in the liftitself, but later commits collapse
Scope/RecallScopeand dropScope.SessionID(see breaking changes below).WithLLM,WithExtractor,WithJobQueue,WithJobTimeout,WithSoftMerge,WithSweeper, …context.WithTimeoutderived from acancellable
workerCtx;Close()cancels in-flight extractor callsso it doesn't hang forever on a stuck LLM.
Complete/Fail/Reschedule) runs on a separateBackground-rooted ctx so a worker cancel never orphans a successful
job in
running.AuditableandJobControllersub-interfaces (Memory staysminimal; capabilities discovered via type-assert).
Retrieval (
sdk/retrieval)Embed → MultiRetrieve → RRFFusion → Boost → Decay → Rerank → Limit) with explicitState.Upsert/Deleteevents; supportsrecall.Rollbackandrecall.History.entry + a
SupersededByedge rather than deleting); a multiplicativeSupersededDecayranks the predecessor below the survivor withoutlosing it.
Async Save infrastructure
recall.JobQueueinterface + in-processMemoryJobQueue.sdkx/recall/jobqueue/sqlitefor crash-recoverable async Save.SaveAsyncreturns immediately with aJobID;JobControllerexposes
JobStatus/AwaitJob.Observability (production-ready)
OTel coverage now matches what
sdk/history/archive+compactoralreadyhad. New telemetry for the recall + retrieval surface:
`retrieval.stage.` (one per pipeline stage, automatically
parented).
`add_total{outcome,reason}`, `add_duration`,
`recall_total{outcome,reason}`, `recall_duration`, `recall_hits`,
`job_total{outcome=succeeded|failed|timeout|dead, stage, attempts}`,
`job_duration`, `job_lease_errors_total`.
failure (timeout vs error split via `errors.Is(ctx.DeadlineExceeded)`),
dead-letter, bookkeeping failure.
as attributes (PII + cardinality). Only structural shape (`top_k`,
`hits`, `query_len`, `has_user_id`).
Bench (
bench/)packages and stay opt-in).
Build / CI
`go vet` in any submodule was silently swallowed by the `for ... done`
exit code, making "make ci green" occasionally untrue. Bench was
silently failing vet ever since it became its own module.
Breaking changes — please read
Schema: `recall.Scope` no longer has `SessionID`.
`deterministicEntryID` no longer mixes the session line, so re-ingest
may produce duplicates of facts extracted before this change.
Treat the LTM index as a fresh corpus. Re-ingest from the source of
truth or accept the duplication.
API: The `MemoryAware` / `ContextAssembler` glue is deleted.
Compose `history.Memory` + `recall.Memory` at the call site.
Reference: `examples/chatbot-with-recall/main.go` (~120 LoC, full
working example).
Construction: `memory.NewWithLLM(cfg, ...)` (struct config) →
`history.NewBuffer` / `history.NewCompacted` and `recall.New`
(functional options). Old fields like `Type`, `LongTerm.Enabled`,
etc. have no equivalent — composition is now explicit.
Lifecycle: `recall.Memory.Close()` must be called on shutdown
to drain async workers / sweeper. `history.Closer` is opt-in via
type-assert (NewBuffer is stateless, NewCompacted is not).
Imports:
Migration cheat-sheet
```go
// Before
mem, _ := memory.NewWithLLM(memory.Config{
Type: "lossless",
LongTerm: memory.LongTermConfig{Enabled: true, TopK: 5},
}, store, llmClient, ltStore, memory.WithWorkspace(ws))
msgs, _ := mem.Load(ctx, convID) // implicit LT injection
// After
hist := history.NewBuffer(history.NewInMemoryStore(),
history.WithBufferMax(20))
mem, _ := recall.New(memidx.New(),
recall.WithLLM(llmClient),
recall.WithJobQueue(recall.NewMemoryJobQueue()),
recall.WithJobTimeout(2*time.Minute),
)
defer mem.Close()
defer func() { if c, ok := hist.(history.Closer); ok { c.Close() } }()
hits, _ := mem.Recall(ctx, scope, recall.Request{Query: userMsg, TopK: 5})
ltContext := formatHits(hits) // caller decides format
msgs, _ := hist.Load(ctx, convID, history.Budget{})
prompt := ltContext + msgs // caller composes
```
Verification
including the new `TestCloseCancelsStuckJob` regression test that
pins the per-job timeout fix.
goroutine racing with `t.TempDir` cleanup); now uses
`history.Closer` to drain — green at `-count=5`.
Compatibility with main's recent work
was `sdkx/go.mod` (sdk pin v0.1.11 → v0.1.12), auto-resolved by
`ort` strategy.
`sdk/history` and `sdk/recall` do not import `sdk/event` — they're
data layers, event bus is for orchestration (`kanban`, `graph/executor`).
Verified by import graph audit; this was true before the refactor too.
Out of scope (follow-up issues welcome)
do this incrementally without breaking callers).
(current v0 ordering: inner Upsert/Delete succeeds before `Journal.Record`,
acceptable for dev, recommend WAL for production).
RRF tuning).
Made with Cursor