fix(engine): bound Remember/Add prompt budget + per-stage timings (#41) - #44
Merged
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses /remember latency regressions in the extraction prompt assembly by bounding the prompt inputs (existing similar memories + conversation context) and adding per-stage timing observability in Engine.Add().
Changes:
- Add prompt-budget helper functions to cap
conversationCtxandexistingMemoriesby byte budget, plus per-memory content trimming with ellipsis. - Replace hardcoded similarity search parameters (
k=10,minScore=0.5) with newEngineConfigtunables for extraction-neighbor selection. - Emit a structured, per-stage timing log line around the extraction path in
Engine.Add().
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| engine/engine_add_prompt_budget.go | Adds pure helper functions to format and byte-cap extraction prompt components. |
| engine/engine_add_prompt_budget_test.go | Adds unit tests covering budget trimming, nil-safety, ordering, and per-memory trimming behavior. |
| engine/engine_add.go | Integrates prompt budgeting, config-driven similarity search knobs, and stage timing logging into Add(). |
| engine/engine.go | Extends EngineConfig with extraction prompt budget/tunable fields and applies defaults in NewEngine(). |
Comment on lines
+42
to
+44
| ExtractionExistingMaxBytes int // byte cap on formatted existingMemories block (default: 4096; 0 = unlimited) | ||
| ExtractionContextMaxBytes int // byte cap on formatted conversationCtx block (default: 4096; 0 = unlimited) | ||
| ExtractionMemoryContentTrim int // per-memory content trim for the existing-memories prompt (default: 240; 0 = no trim) |
Comment on lines
+66
to
+67
| if perMemContentTrim > 0 && len(content) > perMemContentTrim { | ||
| content = content[:perMemContentTrim] + "..." |
Comment on lines
83
to
153
| @@ -106,26 +108,46 @@ func (e *Engine) Add(ctx context.Context, entityID string, req AddRequest) (*Add | |||
| visibilityFor = &storage.VisibilityContext{AgentID: agentID, TeamID: req.TeamID} | |||
| } | |||
|
|
|||
| similarMemories, err := e.store.FindSimilarWithOptions(ctx, queryEmbedding, entityID, 10, 0.5, storage.SimilarityOptions{ | |||
| AgentID: req.AgentID, | |||
| VisibilityFor: visibilityFor, | |||
| }) | |||
| t0 = time.Now() | |||
| similarMemories, err := e.store.FindSimilarWithOptions( | |||
| ctx, queryEmbedding, entityID, | |||
| e.config.ExtractionExistingMaxCount, | |||
| e.config.ExtractionExistingMinScore, | |||
| storage.SimilarityOptions{ | |||
| AgentID: req.AgentID, | |||
| VisibilityFor: visibilityFor, | |||
| }, | |||
| ) | |||
| if err != nil { | |||
| return nil, fmt.Errorf("failed to find similar memories: %w", err) | |||
| } | |||
| similarMs := time.Since(t0).Milliseconds() | |||
|
|
|||
| var existingMemories []string | |||
| for _, sm := range similarMemories { | |||
| existingMemories = append(existingMemories, fmt.Sprintf("[%s, %s, importance:%.1f] %s", | |||
| sm.Memory.Type, sm.Memory.State, sm.Memory.Importance, sm.Memory.Content)) | |||
| } | |||
| existingMemories, existingBytes := buildExistingMemoriesBlock( | |||
| similarMemories, | |||
| e.config.ExtractionExistingMaxBytes, | |||
| e.config.ExtractionMemoryContentTrim, | |||
| ) | |||
|
|
|||
| // Extract memories using LLM | |||
| t0 = time.Now() | |||
| extractResp, err := e.provider.ExtractMemories(ctx, llm.ExtractionRequest{ | |||
| Content: req.Content, | |||
| ConversationCtx: conversationCtx, | |||
| ExistingMemories: existingMemories, | |||
| }) | |||
| extractMs := time.Since(t0).Milliseconds() | |||
|
|
|||
| // Per-stage timing + payload-size trace. See harness/40-invariants/observability.md | |||
| // (Add() stage timings) and issue #41. Structured key=value so ops/tooling can grep. | |||
| log.Printf( | |||
| "INFO [remember/add] stage=extract entity_id=%q session_ms=%d embed_ms=%d similar_ms=%d extract_ms=%d existing_count=%d existing_bytes=%d ctx_msgs=%d ctx_bytes=%d err=%v", | |||
| entityID, sessionMs, embedMs, similarMs, extractMs, | |||
| len(existingMemories), existingBytes, | |||
| len(conversationCtx), ctxBytes, | |||
| err, | |||
| ) | |||
|
|
|||
| if err != nil { | |||
| return nil, fmt.Errorf("extraction failed: %w", err) | |||
| } | |||
tyejcoleman
force-pushed
the
fix/remember-latency-prompt-budget
branch
from
April 24, 2026 04:58
bf66aed to
0134c59
Compare
Closes #41. Before: Engine.Add() called FindSimilarWithOptions with hardcoded k=10 and minScore=0.5, then verbosely formatted every neighbor into the extraction prompt with no byte/token cap. Repeated same-entity writes accumulated more 0.5+ similarity neighbors, inflating the prompt until local LLMs (qwen2.5:7b-instruct) degraded to 25-55s per call and hit the client 60s timeout. /search stayed fast (~40ms) and raw Ollama generate stayed fast (~6-8s), confirming the bottleneck lives in the Add() context assembly, not the provider baseline. Changes: - EngineConfig gains ExtractionExistingMaxCount (k), ExtractionExistingMinScore, ExtractionExistingMaxBytes (4KB default), ExtractionContextMaxBytes (4KB default), ExtractionMemoryContentTrim (240 chars default). NewEngine applies defaults on zero values; negative values opt out (unlimited). - New helpers buildConversationContext and buildExistingMemoriesBlock trim by total byte budget while preserving the most-similar-first ordering that FindSimilarWithOptions returns. Per-memory content is trimmed with an ellipsis so a single verbose memory can't blow the budget on its own. - Engine.Add() now uses the configured k/minScore instead of literals, and emits one structured log line per call with session_ms, embed_ms, similar_ms, extract_ms, existing_count/bytes, ctx_msgs/bytes, err. This matches the new Add() stage-timing invariant in harness/40-invariants/observability.md and gives us a grep-able regression signal without adding Prometheus/OTel. Validation: - Unit: 6 tests in engine_add_prompt_budget_test.go cover unlimited, byte-budget trim, per-memory content trim, nil safety, and most-similar-first preservation. - Full ./engine/... suite still green.
tyejcoleman
force-pushed
the
fix/remember-latency-prompt-budget
branch
from
April 24, 2026 05:03
0134c59 to
f75a369
Compare
Contributor
Author
|
I have read the CLA Document and I hereby sign the CLA |
Contributor
Author
|
recheck |
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.
Summary
Closes #41.
existingMemoriesandconversationCtxby byte budget (4KB each default) before the extraction LLM call; trim per-memory content to 240 chars with ellipsis.k=10, minScore=0.5inengine_add.go:109withEngineConfig.ExtractionExistingMaxCount/ExtractionExistingMinScore.Add()call withsession_ms,embed_ms,similar_ms,extract_ms,existing_count/bytes,ctx_msgs/bytes,err— required by the new invariant inharness/40-invariants/observability.md.Why
moltar-bot's profile (issue #41) showed
/rememberdegrading to 25–55s p95 on repeated same-entity writes under Ollama +qwen2.5:7b-instruct, with a 60s client timeout hit./search(~40ms) and raw Ollamagenerate(~6–8s) stayed fast, localizing the bottleneck to extraction prompt assembly. The root is inengine_add.go:109-128: every 0.5+ similarity neighbor was appended verbosely to the prompt, with no byte cap. Harness45-tunables.mdalready flaggedk=10, minScore=0.5as hardcoded — fixing it here closes that gap.Harness changes (companion)
45-tunables.md— new "Extraction prompt budget (Add / Remember)" table.40-invariants/observability.md— new "Required: Add()/Remember stage timings" subsection defining the mandatory log line.Validation
engine/engine_add_prompt_budget_test.go— unlimited, byte-budget trim (context + existing), per-memory content trim, nil safety, most-similar-first preservation.go test ./engine/ -run TestBuildConversation\|TestBuildExistingMemoriesBlock— all pass.go test ./engine/... -count=1— full suite green.go vet ./... && go build ./...— clean.Deferred (separate tickets)
state=pending_extractionasync path or a reduced-prompt "lite" fallback.Test plan
existingMemoriesunder pressure./rememberon Ollama qwen2.5:7b) and compare p95 before/after. Expect extract_ms drop proportional to the existing-memories block shrink.stage=extractlog lines in a live run to confirm the structured timing invariant is intact.🤖 Generated with Claude Code