Repository navigation
feat: async write-path enrichment — decouple ingest from LLM latency (630× on the hot path) - #117
Conversation
Decouple the fast raw write from slow LLM enrichment: store() enqueues, enrich_pending()/background worker runs extract->resolve->supersede->fact-store with bounded concurrency on Arc-shared handles. state becomes Arc<RwLock> for full consistency. Default Inline (unchanged); Deferred/Background opt-in. Removes the ~5.7s/turn write-path bottleneck. Measure LoCoMo ingest speedup. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
6 TDD tasks: Arc<RwLock<AgentState>> refactor, EnrichmentMode config + queue, enrich_one engine on shared handles, enrich_pending bounded-concurrent, background worker + shutdown/wait, and the LoCoMo ingest-speedup measurement. APIs verified against current code. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…otal backend-bound) LoCoMo conv-0, 7B reasoner: synchronous ingest 1433.9s vs deferred raw-write 2.3s (630x on the write hot path — store() decoupled from LLM latency). Total enrich speedup 1.1x, backend-bound: ollama default parallelism serves the 8 concurrent requests serially; concurrency proven (16 in-flight) but realized only with a parallel-capable backend. Default Inline unchanged; Deferred/Background carry it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…light tracking + doc/dead-field cleanup - Worker loop now drains before checking the shutdown flag, so a store() racing shutdown() can no longer strand a coalesced Notify permit and park the worker forever. - wait_for_enrichment() now waits on an in-flight counter in addition to the queue, since drain_and_enrich empties the queue into a local batch before enrichment actually finishes. - Documented that Deferred/Background fact-memories lack a KG node, temporal tracking, and analytics events (unlike Inline), in both enrichment.rs and the design spec. - Removed the unused EnrichmentParams::exclude_raw_default field. - Added regression tests for both races. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cb65b2f44
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| self.enrichment_queue.lock().await.push_back( | ||
| memory::enrichment::PendingEnrichment { | ||
| key: key.to_string(), | ||
| value: value.to_string(), | ||
| }, |
There was a problem hiding this comment.
Coalesce stale pending entries before enrichment
When a caller updates the same key more than once before a Deferred batch runs (or before the Background worker drains it), this appends a separate snapshot for each version. enrich_one later processes the old value but applies its extracted relations and ::fact0 data to the current key, whose raw storage entry has already been overwritten; the resulting graph/fact memory can therefore describe an obsolete version. Replace or version pending work per key, or verify the queued value is still current before applying enrichment.
Useful? React with 👍 / 👎.
| pub async fn stats(&self) -> MemoryStats { | ||
| let state_guard = self.state.read().await; |
There was a problem hiding this comment.
Keep existing query methods synchronous
This changes the public stats() API from returning MemoryStats immediately to returning a future; the same state refactor also changes public has_memory() to async. Existing downstream users of the 0.2.0 API (for example, memory.stats().total_size or if memory.has_memory(key)) will no longer compile even though the crate version was not bumped. Preserve compatible synchronous query access or make this a deliberate major-version API change.
Useful? React with 👍 / 👎.
Decouples the fast raw
store()from slow per-turn LLM enrichment — the architectural bottleneck that made write-time distillation and LLM-supersession detection impractical (ingest gated on ~5.7s/turn LLM calls). Spec + plan underdocs/superpowers/.What
AgentMemory.state→Arc<RwLock<AgentState>>(behavior-preserving foundation).MemoryConfig::enrichment_mode:Inline(default, unchanged) /Deferred/Background. In Deferred/Background,store()records the raw memory + enqueues; enrichment (extract → resolve → supersede → fact-store) runs later.enrich_oneengine (src/memory/enrichment.rs): the existing enrichment logic lifted onto sharedArchandles; feeds storage + state + the scoring provider so facts stay retrievable; preserves the failure-ordering invariant.enrich_pending(): bounded-concurrent drain (default 8) →EnrichmentReport, best-effort.Backgroundworker +shutdown()/wait_for_enrichment()(in-flight-tracked for determinism).Measurement (LoCoMo conv-0, 419 turns, local 7B reasoner)
The write hot path drops 1434 s → 2.3 s (~630×) —
store()is fully decoupled from LLM latency. InBackgroundmode enrichment runs off the critical path entirely.Honest caveat: the total enrichment speedup is 1.1× — a single 7B model on one GPU (ollama, default
num_parallel≈ 1–2) serves the 8 concurrent requests nearly serially, so the LLM backend, not the write path, is now the ceiling. The concurrency mechanism is proven (16 in-flight connections during the deferred enrich); realizing it on total time needs a parallel/batching-capable backend (OLLAMA_NUM_PARALLEL=8, replicas, or a hosted API). Documented indocs/evaluation.md.Quality
6 TDD tasks, per-task spec+quality reviews. The per-task review found and fixed a real deadlock in
retrieve()(anif letwrite-guard held across a promotion re-lock — passed 484 tests but hung on promotion-eligible retrieval; now regression-tested). The final whole-branch review found ashutdown()hang + await_for_enrichment()early-return (both fixed with regression tests). 493 lib tests pass (no hangs), clippy--all-targetsclean, fmt clean, deadlock-free (all lock sites traced),Inlinedefault byte-for-byte unchanged.Documented divergence: Deferred/Background fact-memories are retrievable via storage/dense/sparse search and checkpointable, but (unlike Inline) don't get a KG node / temporal / analytics — noted in the spec and
store_fact_shared.🤖 Generated with Claude Code