Skip to content

perf(memory): remove sync LLM from AddFact; background consolidation at session end#13

Merged
jkyberneees merged 2 commits into
mainfrom
feat/memory-async-merge
Jun 6, 2026
Merged

perf(memory): remove sync LLM from AddFact; background consolidation at session end#13
jkyberneees merged 2 commits into
mainfrom
feat/memory-async-merge

Conversation

@jkyberneees
Copy link
Copy Markdown
Contributor

Problem

AddFact previously triggered 1–2 synchronous LLM calls inside a tool-call response when merge-on-write classified a new fact as "merge" (cosine ≥ 0.7) or "judge" (borderline, 0.3–0.7). This stalled the agent loop visibly — the user saw the turn hang while judgeMerge + mergeEntries each waited for a network round-trip. With extract_facts now opt-in, manual memory add calls are the primary trigger, so every explicit fact add could hit this latency spike.

Solution

AddFact is now LLM-free:

  • "merge" case → use the fast simple merge (mergeEntries(nil,...)) — handles the common substring/overlap case well; falls back to concatenation otherwise.
  • "judge" case → falls through to "add" without blocking on a judgment call.
  • judgeMerge method removed (now unused).

LLM merge quality recovered at session end: new memory.consolidate_on_end config (default true) runs Consolidate("user") and Consolidate("env") in a background goroutine at session end — the quality complement to merge-on-write:

  • merge-on-write: catches obvious duplicates immediately (zero latency, simple string merge)
  • consolidation: handles near-duplicates and paraphrases once per session with full LLM quality

Config

New: memory.consolidate_on_end (default true). Requires llm_consolidate: true. Runs after episode extraction in the same session-end goroutine.

Verification

  • gofmt/build/vet/-race clean.
  • Tests: AddFact fires zero LLM calls even when merge fires; simple merge fallback produces non-empty result; consolidate_on_end=true reduces fact count at session end (background goroutine ran); consolidate_on_end=false leaves facts untouched.

🤖 Generated with Claude Code

jkyberneees and others added 2 commits June 6, 2026 12:33
…at session end

AddFact previously triggered 1-2 synchronous LLM calls inside a tool-call
response when merge-on-write classified a new fact as "merge" (cosine ≥ 0.7)
or "judge" (borderline). This stalled the agent loop visibly — the user saw
the turn hang while judgeMerge + mergeEntries each waited for a network
round-trip.

AddFact is now LLM-free:
- "merge" case: uses the fast simple merge (mergeEntries(nil,...)) — handles
  the common substring/overlap case well; concatenates for the rest.
- "judge" case: falls through to "add" without blocking on a judgment call.
- judgeMerge removed (unused).

LLM merge quality is recovered at session end by a new background consolidation
step: when consolidate_on_end=true (default) and llm_consolidate=true, the
session-end goroutine runs Consolidate("user") and Consolidate("env") after
episode extraction. This is the quality complement to merge-on-write — simple
merge catches obvious duplicates immediately (zero latency); consolidation
handles near-duplicates and paraphrases once per session with full LLM quality.

New config: memory.consolidate_on_end (default true), wired through MemoryConfig,
DefaultMemoryConfig, NewMemoryManager merge, and resolveMemory.

Tests: AddFact fires zero LLM calls even when merge fires; simple merge
fallback produces non-empty result; consolidate_on_end=true reduces fact count
at session end; consolidate_on_end=false leaves fact count stable.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…/D-06/D-08/D-09)

D-06 (critical): consolidate_on_end was gated behind the LLMExtract early-return
in OnSessionEndWithProvenance, silently disabling consolidation whenever a user
set llm_extract=false (to disable episode extraction). These features are
conceptually independent. Fix: move the consolidation goroutine launch before the
LLMExtract guard, with its own independent gate (llm != nil, turns >= min,
ConsolidateOnEnd=true, LLMConsolidate=true). Verified: consolidation fires with
llm_extract=false; added regression test.

D-03: Consolidate had no upper-bound guard on LLM-returned entries; a
hallucinating LLM could expand the entry count. Fix: cap newEntries to
len(entries) before writeEntries.

D-02: writeEntries doc comment said "Caller must hold f.mu" but Consolidate
calls it holding factsDirLock instead of f.mu (safely, but undocumented). Fix:
clarify the locking contract — either f.mu (normal paths) or factsDirLock
(Consolidate) is sufficient; the two are never acquired together.

D-08: CHEATSHEET.md still documented the old judge behavior ("LLM judges →
merge or add"). Updated to reflect the new behavior (auto-add, deferred to
session-end consolidation).

D-09: CONFIG.md noted consolidate_on_end as the complement to merge-on-write,
but did not warn that disabling it causes near-duplicates to accumulate
permanently. Added an explicit note.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@jkyberneees
Copy link
Copy Markdown
Contributor Author

Verification Protocol (v5.2.7) — Remediation pass (commit 6034ba0)

All findings closed.

Finding Sev Status
D-06 consolidate_on_end silently disabled when llm_extract=false CRITICAL ✅ Fixed — consolidation goroutine moved outside the LLMExtract guard, with its own independent gate. Verified with injection test + permanent regression guard.
D-03 No upper-bound on LLM-returned entries (hallucinating LLM could expand facts) WARN ✅ Fixed — newEntries capped to len(entries) before writeEntries.
D-02 writeEntries locking contract unclear (factsDirLock vs f.mu) WARN ✅ Documented — clarified that either lock is sufficient; the two are never acquired together.
D-08 CHEATSHEET.md still described old judge behavior (LLM judges → merge or add) WARN ✅ Updated to reflect auto-add + deferred session-end consolidation.
D-09 consolidate_on_end=false now causes permanent near-duplicate accumulation (undocumented regression) WARN ✅ Documented in CONFIG.md.
D-01 No deadlock/livelock between Consolidate + AddFact info ✅ Confirmed held
D-04 Consolidate holds lock during LLM call (latency displaced) WARN Accepted — blocks other sessions' AddFact, not the requesting user; this is strictly better than before.
D-05 Double-fire on simultaneous serve.go disconnects info Accepted — idempotent; second Consolidate hits early-exit at ≤1 entry
D-07 Early-exit path for 0/1 entries verified info ✅ Confirmed held

Verdict: 🟡 HumanReviewRequired — monoculture only

No axis is 🔴. Every substantive finding is resolved. The remaining floor is the monoculture ρ penalty (same model authored and verified).

@jkyberneees
Copy link
Copy Markdown
Contributor Author

Verification Certificate — Before & After

Protocol v5.2.7 · PR #13 perf(memory): remove sync LLM from AddFact; background consolidation at session end


🔴 BEFORE — commit 09582a1 (initial submission)

Classification: GeneratedCode · Pipeline: monoculture (Anthropic) · η ≈ 0.60 · ρ = 0.21

Axis Finding
2.1 Semantic Correctness ⚠️ D-06: consolidate_on_end silently does nothing when llm_extract=false — stated quality-recovery behavior does not work. D-03: LLM can expand fact count (no upper-bound guard).
2.2 Behavioral Contract ⚠️ C-18 violated: contract says consolidation is independent of llm_extract; implementation gates it behind the same shared early-return.
2.3 Security
2.4 Structural Integrity ⚠️ D-02: writeEntries doc contract says "Caller must hold f.mu" but Consolidate uses factsDirLock instead — undocumented invariant.
2.5 Behavioral Exploration ⚠️ D-04: consolidation holds factsDirLock during LLM call, blocking concurrent AddFact from other sessions — latency displaced, not eliminated.
2.6 Dependency Integrity
2.7 Generator Provenance ⚠️ Monoculture
2.8 Adversarial Surface
2.9 Documentation Coverage 🔴 D-08: CHEATSHEET.md still documents the old judge behavior ("LLM judges → merge or add") — factually wrong after this PR. D-09: no documentation that consolidate_on_end=false causes permanent near-duplicate accumulation, a behavioral regression.

Verdict: 🔴 HumanReviewRequired — axis 2.9 at 🔴 (stale doc + undocumented regression), axis 2.1 material gap (D-06), η 0.60 < 0.80, ρ 0.21 ∈ (0.20, 0.30].


✅ AFTER — commit 6034ba0 (post-remediation)

η ≈ 0.68 · ρ = 0.21 (unchanged — monoculture)

Axis
2.1 Semantic Correctness D-06 fixed (independent gate). D-03 fixed (entry-count cap).
2.2 Behavioral Contract C-18 now holds: consolidation fires regardless of llm_extract.
2.3 Security
2.4 Structural Integrity D-02 resolved: locking contract documented.
2.5 Behavioral Exploration ⚠️ D-04 accepted — blocking other sessions' AddFact is strictly better than blocking the requesting user's turn (pre-PR behavior).
2.6 Dependency Integrity
2.7 Generator Provenance ⚠️ Monoculture (unchanged)
2.8 Adversarial Surface
2.9 Documentation Coverage D-08 fixed (CHEATSHEET.md updated). D-09 documented in CONFIG.md.

Verdict: 🟡 HumanReviewRequired — monoculture only. No axis is 🔴. Every substantive finding is resolved. The remaining floor is the ρ penalty (same model authored and verified) — cannot be cleared without a human reviewer or independent second-provider pass.


Delta

Before After
Axes at 🔴 1 (2.9) 0
Axes at ⚠️ 4 2 (monoculture + accepted D-04)
η ≈ 0.60 ≈ 0.68
Binding verdict gate axis 2.9 🔴 + D-06 + η ρ (monoculture) only

@jkyberneees jkyberneees merged commit cb83cf7 into main Jun 6, 2026
6 checks passed
@jkyberneees jkyberneees deleted the feat/memory-async-merge branch June 6, 2026 10:48
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.

1 participant