fix(memory): MEDIUM follow-ups — counter map sweep, hot-reload on PATCH, multi-keyword search, configurable UPDATE thresholds#5850
Merged
Conversation
…CH, multi-keyword search, configurable UPDATE thresholds Continuation of the audit sweep on the proactive-memory subsystem. 4 MEDIUM findings from the same audit, all in scope for this PR. * M11 `consolidation_counters` HashMap is now actively pruned every maintenance tick. Pre-fix it only swept when the map crossed 1000 entries (truncate-to-500 by count DESC), which delayed cleanup until the leak was observable AND deleted the highest-count entries — exactly the agents about to fire a real consolidate. The new sweep drops every counter that hasn't passed the halfway mark (`< AUTO_CONSOLIDATE_EVERY / 2 = 5`) on each maintenance tick: HashMap::retain in-place, evicts cold entries first, never touches an entry that's about to fire. Added named constant `AUTO_CONSOLIDATE_EVERY = 10` so the trigger + the prune floor stay in lockstep. * M12 `PATCH /api/memory/config` now calls `kernel.reload_config()` after writing `config.toml`, so dashboard saves take effect on the running kernel instead of staying disk-only until restart. Pre-fix the response always reported `restart_required: true`, which confused operators who could see GET return the new values while live behaviour (ProactiveMemoryStore::config, decay engine, etc.) stayed on the boot snapshot. `restart_required` now reflects the actual `ReloadPlan` — false when every diff field hot-reloads, true when any field needs a restart. A reload validation failure is surfaced via the new `reload_error` field instead of swallowing the disk write. * M13 `extract_search_keywords` returns a `Vec<String>` of the top 4 distinctive keywords ordered longest-first (post-stop-word filter, post-dedup), and the caller iterates over them unioning per-keyword LIKE recalls until `fetch_limit` is hit. Pre-fix it collapsed the four candidates to the single longest one, wasting the stop-word filter work on the other three and giving the no-embedding fallback path a frequently-too-generic substring (e.g. "analysis") to match against, OR a too-specific compound term that matched nothing. The fallback to the raw-content LIKE is preserved for the no-distinctive-words case so a near-verbatim duplicate is still detectable. * M14 the `decide_action` UPDATE thresholds are now configurable via two new `ProactiveMemoryConfig` fields: `update_threshold_same_category` (default 0.7) and `update_threshold_cross_category` (default 0.8). The trait method signature stays stable — `add_with_decision` stashes the live config values in the new memory's metadata under `_update_threshold_same_cat` / `_update_threshold_cross_cat`, the default `decide_action` reads them out (falling back to the const defaults for direct trait-method callers), and the LLM-backed extractor inherits the same behaviour via its fallback to the default heuristic on driver failure. This separates the per-insertion conflict-resolution threshold (UPDATE vs ADD) from the post-hoc consolidation threshold (`duplicate_threshold`) — pre-fix both were conflated. Drive-by fmt: two stale formatting hunks `cargo fmt` flagged in `runtime/tool_runner/wasm_skill.rs:159` and `api/tests/memory_routes_integration.rs:518` (neither mine, both inherited from main). Verification: * cargo check --workspace --lib — clean * cargo clippy -p librefang-memory -p librefang-runtime -p librefang-types -p librefang-api -p librefang-kernel --tests -- -D warnings — clean * cargo test -p librefang-memory --lib — 273 passed (4 new regression tests: `extract_search_keywords_returns_multiple_ordered_longest_first`, `extract_search_keywords_empty_for_all_stop_words`, `decide_action_honors_config_update_thresholds`, and the existing suite re-verified against the new `update_threshold_*_category` config fields) * cargo test -p librefang-runtime --lib proactive_memory — 60 passed * cargo test -p librefang-api --lib --test memory_routes_integration --test agent_kv_authz_integration --test auth_public_allowlist — all green
houko
force-pushed
the
fix/memory-critical-bundle
branch
from
May 29, 2026 01:08
35a1119 to
cb107cc
Compare
added 4 commits
May 29, 2026 10:21
…loor, partial-status, dedup-strip, test tightening 8 follow-ups from the code review of the prior commit. All in scope for the same proactive-memory layer. * #1 misattributed doc-comment in `proactive.rs` near `AUTO_CONSOLIDATE_EVERY` / `NEGATION_WORDS`. The `/// Negation/contradiction words …` line was orphaned above `AUTO_CONSOLIDATE_EVERY` when the new const got inserted; both consts now carry their own intended docstring. * #2 lowered `STALE_COUNTER_FLOOR` from `AUTO_CONSOLIDATE_EVERY / 2` (5) to `/ 4` (2). The /2 floor cleaned up "stuck at 1..4" agents but also reset slow-burn agents (single auto_memorize per maintenance tick) before they could climb to 10, so a steady 1-call/hour stream effectively never consolidated. /4 keeps the cold-slot eviction directional while letting low-frequency agents still accumulate to the trigger. * #3 `PATCH /api/memory/config` response shape is now explicit about partial success. `body.status` is `"applied"` when the reload succeeded and `"partial"` when the disk write landed but the live reload failed (e.g. operator hand-edited an unrelated section into an invalid shape between PATCH writes). Clients MUST inspect `status`; the HTTP code stays 200 for both branches since the disk write itself succeeded. 207 / 500 were considered and rejected — 500 misrepresents that the request was rejected (it wasn't) and 207 forces every existing client to re-classify success. Mirrors the `import_agent_memory` partial-success pattern. * #4 documented the M13 cost tradeoff. Up to 4 SQLite roundtrips per insertion on the no-embedding fallback path (versus the pre-fix 1), but the embedding-driver path is unaffected and the loop short-circuits as soon as the union hits fetch_limit, so the common case ("first keyword filled the slate") still runs a single query. Operators on the no-embedding path pay the cost on writes only, against an already-unindexed `content LIKE` scan. * #5 defensively strip the M14 `_update_threshold_*` keys from the enriched item right after `decide_action` returns + assert callers don't pre-populate them. Both insertion branches today build their stored metadata from the original `item.metadata` (not `enriched_item.metadata`), so the threshold keys never reach the column — but stripping + asserting is cheap insurance for the next refactorer who repoints either branch at the enriched copy. * #6 relaxed the `extract_search_keywords_returns_multiple_ordered_longest_first` test: no more exact `kws.len() == 4` — the cap and the "longest distinctive word survives" invariants are what we care about, not the exact count. Future additions to `STOP_WORDS` no longer silently break the test. * #7 dropped the unused `_update_threshold_cross_cat` set in `decide_action_honors_config_update_thresholds`. Both candidate memories share the "preference" category, so the cross-cat threshold branch was unreachable. * #8 moved `STALE_COUNTER_FLOOR` from a `fn`-scope `const` to module scope, alongside `AUTO_CONSOLIDATE_EVERY` from which it derives. Matches the repo convention and lets future readers see the floor / trigger relationship at one site. Verification: * cargo check --workspace --lib — clean * cargo clippy -p librefang-memory -p librefang-runtime -p librefang-types -p librefang-api --tests -- -D warnings — clean * cargo test -p librefang-memory --lib — 273 passed * cargo test -p librefang-api --test memory_routes_integration — 14 passed
… + M14 regression coverage, comment polish 5 follow-ups from the second review pass on #5850. * B (was #1) M11 done properly. `consolidation_counters` is now `HashMap<String, CounterEntry>` where `CounterEntry` carries both the running count and a `last_touched: DateTime<Utc>` stamp. The maintenance sweep evicts entries whose `last_touched` is older than `STALE_COUNTER_IDLE_WINDOW = 2 hours` (~2× the maintenance rate-limit window), regardless of count. The previous count-threshold fix (followup #2 in the prior commit) mitigated but didn't solve the slow-burn case: any agent firing ≤ 1 × per maintenance window would still be reset before climbing past the count floor. The timestamp-based check closes that gap — an active slot, however slow, is preserved as long as it's been touched within the window; a truly idle slot is reclaimed within ~2 hours of going quiet. * C (was #2) `PATCH /api/memory/config` happy-path test added at `memory_routes_integration::patch_memory_config_hot_reloads_and_reports_applied`. Pre-seeds a minimal `config.toml` (the harness's tempdir previously didn't materialise one, which the file-level docstring flagged as out-of-scope; the docstring updated accordingly) and asserts `body["status"] == "applied"`, `body["reload_error"]` null, and that the PATCHed value round-trips into the response. Without this, the M12 status contract could silently revert and the rest of the suite wouldn't catch it. `RouterHarness._tmp` is exposed as `tmp` to let the test reach the seed location. * D (was #3) M14 strip regression test added at `proactive::tests::add_with_decision_does_not_leak_threshold_keys_to_stored_metadata`. Drives the full `add()` path through `add_with_decision`, then reads back via `list()` and asserts none of the private `_update_threshold_*` / `_embedding` keys leaked into the stored metadata column. Catches the regression where someone repoints the ADD or UPDATE branch at `enriched_item.metadata` (the decision-clone) instead of the original `item.metadata` (the caller's input). * E (was #4) the `debug_assert!` panic messages on the `_update_threshold_*` private keys re-worded from "caller leaked it" to "callers must not pre-populate ..." — neutral phrasing that doesn't presume the caller is buggy. Also added a one-line note that the production path stays safe regardless (the unconditional `insert` overwrites any leaked value before `decide_action` reads it), since `debug_assert!` is compiled out in release. * F (was #5) M13 cost-trade-off comment tightened. The "common case still runs a single query" claim was accurate for agents with sizeable stores (first keyword exhausts fetch_limit) but not for fresh / small stores where no individual keyword has enough matches to short-circuit. The comment now distinguishes the two regimes instead of overgeneralising. Re-stamped `.secrets.baseline` line numbers shifted by the test file edits. Verification: * cargo check --workspace --lib — clean * cargo clippy -p librefang-memory -p librefang-api --tests -- -D warnings — clean * cargo test -p librefang-memory --lib — 274 passed (incl. `add_with_decision_does_not_leak_threshold_keys_to_stored_metadata`) * cargo test -p librefang-api --test memory_routes_integration — 15 passed (incl. `patch_memory_config_hot_reloads_and_reports_applied`)
…epts partial, M14 strips _embedding too, prune rate-limit, chrono::Duration const portability 6 followups from the third review pass on #5850. * #1 M12 test pinned the contract properly. `serde_json::Value` indexed by a missing key returns `Value::Null`, so the prior `assert_eq!(body["reload_error"], Null)` silently passed even if the field had been removed. Now asserts `body.as_object() .contains_key(...)` for `status`, `restart_required`, `reload_error` first, then asserts their values. * #2 strip + test for the `_embedding` private-stash key. `add_with_decision` now also calls `enriched_item.metadata.remove("_embedding")` after `decide_action` returns, so all three private stash keys (`_update_threshold_*` + `_embedding`) get the same defensive treatment. Test renamed to `add_with_decision_does_not_leak_private_stash_keys_to_stored_metadata` and attaches a tiny mock `EmbeddingFn` so the `_embedding` stash path actually fires — the prior assertion against `_embedding` was decorative because the test had no embedding driver configured. * #3 rate-limited the counter prune via a new `last_counter_prune: Arc<Mutex<Option<DateTime<Utc>>>>` and a `maybe_prune_counters` helper that mirrors the `maybe_decay_confidence` / `maybe_cleanup_expired` once-per-hour pattern. Prior to this the prune ran on every `maybe_run_maintenance` call, reachable from `search` / `auto_retrieve` / `consolidate` at potentially many Hz. The retain itself was microseconds, so this is a wash today, but it brings the three maintenance sub-tasks under the same scheduling budget for future scaling. * #4 docstring on `STALE_COUNTER_IDLE_WINDOW_HOURS` re-phrased to describe the slot-keeping guarantee in terms of the maintenance rate-limit window, not "consecutive prune passes" — the old phrasing happened to be true only because the prune wasn't rate-limited yet (now rectified by #3). * #5 M12 test now accepts either `"applied"` or `"partial"` as the body status — both are valid post-fix outcomes; the pre-fix contract had no `status` field at all. The status field's presence (asserted via the #1 fix) is the actual contract we're pinning. Made the test robust to future `KernelConfig::default()` changes that might cause the seeded toml to fail reload validation. * #6 swapped `const STALE_COUNTER_IDLE_WINDOW: chrono::Duration = chrono::Duration::hours(2)` for `const STALE_COUNTER_IDLE_WINDOW_HOURS: i64 = 2` plus `chrono::Duration::hours(STALE_COUNTER_IDLE_WINDOW_HOURS)` at the call site. `chrono::Duration::hours` is a `const fn` at the currently pinned `chrono` minor but the const-ness isn't a stable contract across `0.4.x` versions, so a lockfile bump could silently break the build. The integer-hours + runtime conversion stays valid regardless. Verification: * cargo check --workspace --lib — clean * cargo clippy -p librefang-memory -p librefang-api --tests -- -D warnings — clean * cargo test -p librefang-memory --lib — 274 passed (incl. `add_with_decision_does_not_leak_private_stash_keys_to_stored_metadata` with embedding-driver coverage) * cargo test -p librefang-api --test memory_routes_integration — 15 passed (incl. tighter `patch_memory_config_hot_reloads_and_reports_applied` contract assertions)
…strip helper + non-empty partial-error 3 follow-ups from the fourth review pass on #5850. All three are about closing gaps between what the code does and what the docstrings / tests claim it does. * A `STALE_COUNTER_IDLE_WINDOW_HOURS` docstring: the "reclaimed within ~2 hours of going quiet" claim was accurate before the round-3 prune rate-limit landed, after which the worst-case reclaim latency is 2-3 hours (idle window + up to one prune rate-limit period because the prune itself only runs ≤ once per hour). Re-phrased with explicit upper/lower bounds; deleted the duplicate "previous fix (round-1 followup #2)" paragraph that had ended up in the doc twice during the round-2 edit. * B `strip_private_stash_keys` extracted into a module-level function driven by a single `ADD_WITH_DECISION_PRIVATE_STASH_KEYS` const, and unit-tested directly via `strip_private_stash_keys_removes_all_private_keys`. The integration test `add_with_decision_does_not_leak_private_stash_keys_to_stored_metadata` only catches a *coordinated two-step regression* (strip removed AND ADD/UPDATE branch repointed at `enriched_item.metadata`) — the current ADD path bypasses `enriched_item.metadata` entirely, so single-step regressions of either kind pass that test. The prior commit's docstring claimed "the strip code on the post-decide path is the only thing keeping it out of the stored column", which was wrong — `item.metadata` (the caller's input) is what actually keeps the keys out today. Updated the docstring to match reality and added the direct unit test on the helper to cover single-step strip regressions. * C `reload_error` partial-branch assertion in `patch_memory_config_hot_reloads_and_reports_applied` now also rejects empty / whitespace-only strings. `is_string()` alone passed `""` / `" "` / any other zero-info value — operators would see status=partial with a useless error blob and have no actionable diagnostic. Trimmed-non-empty makes the contract honest about what "carries the validator output" means. Verification: * cargo check --workspace --lib — clean * cargo clippy -p librefang-memory -p librefang-api --tests -- -D warnings — clean * cargo test -p librefang-memory --lib — 275 passed (1 new: `strip_private_stash_keys_removes_all_private_keys`) * cargo test -p librefang-api --test memory_routes_integration — 15 passed
This was referenced May 29, 2026
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.
Follow-up on #5839 — picks up four MEDIUM findings from the same proactive-memory audit that landed too late to make the merge window.
Coverage
consolidation_countersHashMap pruned every maintenance tick (drop entries belowAUTO_CONSOLIDATE_EVERY / 2 = 5), instead of waiting for the 1000-entry truncate-to-500 sweep that both delayed cleanup and dropped the hottest entries first.PATCH /api/memory/confignow callskernel.reload_config()after writingconfig.toml, so dashboard saves take effect live.restart_requiredreflects the actualReloadPlan;reload_errorsurfaces validation failures.extract_search_keywordsreturnsVec<String>of the top 4 distinctive keywords (longest-first); the no-embedding fallback iterates and unions per-keyword recalls. Pre-fix it collapsed to the single longest word, wasting the stop-word filter on the other three.decide_actionUPDATE thresholds split into twoProactiveMemoryConfigfields (update_threshold_same_categorydefault 0.7,update_threshold_cross_categorydefault 0.8). The trait method reads them fromMemoryItem.metadata(whereadd_with_decisionstashes the live values), so the signature stays stable.Verification
cargo check --workspace --lib— cleancargo clippy -p librefang-memory -p librefang-runtime -p librefang-types -p librefang-api -p librefang-kernel --tests -- -D warnings— cleancargo test -p librefang-memory --lib— 273 passed (4 new regression tests)cargo test -p librefang-runtime --lib proactive_memory— 60 passedcargo test -p librefang-api --lib --test memory_routes_integration --test agent_kv_authz_integration --test auth_public_allowlist— all greenNew regression tests:
proactive::tests::extract_search_keywords_returns_multiple_ordered_longest_firstproactive::tests::extract_search_keywords_empty_for_all_stop_wordsproactive::tests::decide_action_honors_config_update_thresholdsDrive-by
Two stale formatting hunks
cargo fmtflagged inruntime/tool_runner/wasm_skill.rs:159andapi/tests/memory_routes_integration.rs:518— both inherited from main, not introduced here.