fix(stats): drop permanently failed memories from pending_consolidation - #3397
Merged
Merged
Conversation
…on (#3362) `pending_consolidation` counted every fact with `consolidated_at IS NULL`, including the ones stamped `consolidation_failed_at` that the consolidator's own candidate query (`reads.find_unconsolidated`) excludes on purpose. The gauge therefore had a floor no amount of work could clear: it sat above `?consolidation_state=pending` by exactly `failed_consolidation`, and an operator could not tell a real backlog from an abandoned residue. `pending` now carries the consolidator's predicate, so the two buckets are disjoint and a bank with no live backlog reaches zero. The same predicate was missing in five more places: - `get_bank_stats` keeps a second copy of the freshness SQL for the `writes_memory_rows_in_sql` path — fixing only `counts.py` would have left the default Postgres path wrong. - `hindsight.consolidation.backlog` had the same floor, which made "backlog > 0 for N minutes" unalertable on any bank holding a residue. - reflect's `tool_search_observations` derives `is_stale` / `freshness` from this count, so a residue told the model the observations were stale on every call, for ever (fixed transitively via `get_bank_freshness`). - the control plane's consolidation card computed `done = total - pending`, which would have counted the failed rows as done once pending got strict. - the benchmark runner waits for this count to reach 0, so one permanently failed fact burned the full 3000s timeout. Everything else that answers "what is left to consolidate" already excluded them: the memories list filter, `count_unconsolidated`, and the `banks_needing_consolidation()` maintenance routine — so scheduling was never spinning on the residue.
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.
Fixes #3362.
The bug
pending_consolidationcounted every fact withconsolidated_at IS NULL, including the ones stampedconsolidation_failed_atthat the consolidator's own candidate query (reads.find_unconsolidated) skips on purpose. The gauge had a floor no amount of work could clear — it sat above?consolidation_state=pendingby exactlyfailed_consolidation, indefinitely — so an operator could not tell a real backlog from an abandoned residue.pendingnow carries the consolidator's predicate. The two buckets are disjoint: a fact the LLM could not handle is counted once, underfailed, and leaves that bucket only viaPOST /consolidation/recover.The same predicate was missing in five more places
get_bank_statskeeps a second copy of the freshness SQL for thewrites_memory_rows_in_sqlpath — fixing onlycounts.pywould have left the default Postgres path wrong.hindsight.consolidation.backloghad the same floor, which made "backlog > 0 for N minutes" unalertable on any bank holding a residue.tool_search_observationsderivesis_stale/freshnessfrom this count (staleat ≥10), so a residue told the model the observations were stale on every reflect call, for ever. Fixed transitively viaget_bank_freshness.done = total - pending, which would have counted the failed rows as done once pending got strict; it now subtracts both, so done / pending / failed no longer overlap.Everything else that answers "what is left to consolidate" already excluded them — the memories list filter,
count_unconsolidated, and thebanks_needing_consolidation()maintenance routine — so scheduling was never spinning on the residue.The issue also names
components/data-view.tsx; that surface no longer exists onmain, so only the stats card needed the render fix.Notes
pending_consolidation's OpenAPI description said "not yet processed into observations", which is what invited the superset reading. It now states the exclusion; specs and clients are regenerated.consolidation_failed_at IS NULLterm is not inidx_memory_units_unconsolidated's predicate, so it is a recheck on rows the index already returned. The failed set is tiny by construction, and the metrics query keeps itsSET LOCAL enable_seqscan = offnudge.Testing
pending_consolidation == GET /memories/list?consolidation_state=pending → total— the exact equality the issue reported as broken — plusfailed_consolidationcounted separately.test_bank_stats_reports_failed_consolidationandtest_get_bank_freshness_...now assert the strict counts instead of>=.test_backlog_metrics.pyasserts the gauge SQL carries the exclusion.test_consolidation_failure_recovery.py+test_maintenance_routines.py(31 passed), andtest_backlog_metrics.py(7 passed) all pass; lint clean.