Skip to content

fix(memory): MEDIUM follow-ups — counter map sweep, hot-reload on PATCH, multi-keyword search, configurable UPDATE thresholds#5850

Merged
houko merged 5 commits into
mainfrom
fix/memory-critical-bundle
May 29, 2026
Merged

fix(memory): MEDIUM follow-ups — counter map sweep, hot-reload on PATCH, multi-keyword search, configurable UPDATE thresholds#5850
houko merged 5 commits into
mainfrom
fix/memory-critical-bundle

Conversation

@houko

@houko houko commented May 29, 2026

Copy link
Copy Markdown
Contributor

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

ID Fix
M11 consolidation_counters HashMap pruned every maintenance tick (drop entries below AUTO_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.
M12 PATCH /api/memory/config now calls kernel.reload_config() after writing config.toml, so dashboard saves take effect live. restart_required reflects the actual ReloadPlan; reload_error surfaces validation failures.
M13 extract_search_keywords returns Vec<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.
M14 decide_action UPDATE thresholds split into two ProactiveMemoryConfig fields (update_threshold_same_category default 0.7, update_threshold_cross_category default 0.8). The trait method reads them from MemoryItem.metadata (where add_with_decision stashes the live values), so the signature stays stable.

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 --lib273 passed (4 new regression tests)
  • cargo test -p librefang-runtime --lib proactive_memory60 passed
  • cargo test -p librefang-api --lib --test memory_routes_integration --test agent_kv_authz_integration --test auth_public_allowlist — all green

New regression tests:

  • proactive::tests::extract_search_keywords_returns_multiple_ordered_longest_first
  • proactive::tests::extract_search_keywords_empty_for_all_stop_words
  • proactive::tests::decide_action_honors_config_update_thresholds

Drive-by

Two stale formatting hunks cargo fmt flagged in runtime/tool_runner/wasm_skill.rs:159 and api/tests/memory_routes_integration.rs:518 — both inherited from main, not introduced here.

@github-actions github-actions Bot added size/XL 1000+ lines changed has-conflicts PR has merge conflicts that need resolution area/docs Documentation and guides area/runtime Agent loop, LLM drivers, WASM sandbox area/kernel Core kernel (scheduling, RBAC, workflows) labels May 29, 2026
…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
houko force-pushed the fix/memory-critical-bundle branch from 35a1119 to cb107cc Compare May 29, 2026 01:08
@github-actions github-actions Bot added size/L 250-999 lines changed ready-for-review PR is ready for maintainer review and removed area/docs Documentation and guides area/kernel Core kernel (scheduling, RBAC, workflows) size/XL 1000+ lines changed has-conflicts PR has merge conflicts that need resolution labels May 29, 2026
Evan 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
@houko
houko merged commit 97bb45d into main May 29, 2026
28 checks passed
@houko
houko deleted the fix/memory-critical-bundle branch May 29, 2026 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/runtime Agent loop, LLM drivers, WASM sandbox ready-for-review PR is ready for maintainer review size/L 250-999 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant