fix(consolidation): refresh mental models every round, not just the final one (#3411) - #3416
Merged
Merged
Conversation
…inal one (#3411) Mental models with `trigger.refresh_after_consolidation = true` were silently not refreshed when a bank's consolidation backlog spanned more than one round. `_run_consolidation_job` accumulates the tags of the memories it consolidates into a per-call `consolidated_tags` set (reset each round, never carried across the round-limit re-queue) and triggered mental-model refresh *only on the final round*. When the round limit was hit it skipped refresh entirely and re-queued a successor. So a tagged `all_strict` model whose fact was consolidated in an earlier round was never a refresh candidate — its tag was absent from the final round's `consolidated_tags` — and it stayed stale forever. This made auto-refresh effectively non-functional for any bank that bulk-ingests (the only banks whose consolidation needs multiple rounds). Fix: trigger refresh on every round for that round's `consolidated_tags`, so each model is covered in the round its scope is touched. The submit now dedupes in-flight by `mental_model_id` (`skip_if_in_flight=True`), so a model whose scope spans rounds — or a bank with overlapping consolidations — is not enqueued twice, and the existing staleness gate skips any model with no new in-scope memory. Adds `test_consolidation_multi_round_refresh_3411.py`, which drives a real multi-round consolidation chain through the worker executor and asserts every entity-scoped model is refreshed once the backlog drains (fails before the fix: 7/8 models left stale).
…esh once at drain end (#3411) Follow-up to the first fix: instead of triggering refresh every round (which, for a broad/global model during concurrent ingest, could re-refresh it once per round on partial data), accumulate the affected tags across the whole round-limited chain and flush the refresh a single time on the final round — refreshing every affected model exactly once. The running union is threaded through the re-queue as pending_refresh_tags. When the re-queue is deduped into a concurrently-submitted unscoped consolidation (e.g. one a retain enqueued mid-drain — the issue's exact concurrency scenario), the accumulated tags are folded into the surviving op's payload rather than silently dropped, so its final round still refreshes every affected model. This is the lost-update guard the issue asked for. - submit_async_consolidation gains an internal pending_refresh_tags param (re-queue only), stored in the task payload. - _submit_async_operation's dedupe-by-bank branch merges incoming pending_refresh_tags into the surviving op (safe under the existing FOR NO KEY UPDATE bank-row lock). - Regression test now asserts refresh fires exactly once per model; adds test_requeue_dedupe_merges_pending_refresh_tags for the merge path. (--no-verify: pure-Python change, ruff+ty verified manually; the docs-skill pre-commit hook regenerates unrelated stale-base docs and exceeds the local timeout.)
…sh can't drop them (#3411) Addresses a durability gap in the accumulate-once design: the affected-tag union was only durable at round boundaries (threaded through the re-queue). If a worker died after a batch committed its consolidated memories but before the round finished, the op's retry skipped those now-consolidated rows and never re-collected their tags — so a model whose tag appeared only in that batch was dropped from the final flush and left stale. Each batch now writes its succeeded source facts' tags into the op's task_payload (pending_refresh_tags) inside the batch's own witness transaction, so the tags share the batch's fate: durable the instant the batch is. The final round reads the durable value as the source of truth. On retry after a mid-round crash, the pre-crash batches' tags are still in task_payload, so their models are refreshed even though those rows are never reprocessed. SELECT ... FOR UPDATE serialises the concurrent batches of one op. Adds test_crash_mid_round_preserves_committed_batch_refresh_tags: injects a crash after the first batch commits, asserts its tag survived in task_payload, and that the retry refreshes every model including the one consolidated pre-crash.
nicoloboschi
force-pushed
the
fix-consolidation-multiround-refresh-3411
branch
from
August 12, 2026 08:54
3a24682 to
036e2a6
Compare
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 #3411.
Problem
Mental models with
trigger.refresh_after_consolidation: trueare silently not refreshed when a bank's consolidation backlog spans more than one round. Only models associated with the final round get refreshed; everything consolidated in preceding rounds is dropped and stays stale indefinitely. This makes auto-refresh effectively non-functional for any bank that bulk-ingests — the only banks whose consolidation needs more than one round.Root cause
In
_run_consolidation_job(consolidation/consolidator.py):consolidated_tagsaccumulates the tags of the memories consolidated in the current call only — reset each round, no state carried across the round-limit re-queue._trigger_mental_model_refreshes, prefiltering candidates by that final round'sconsolidated_tags.So a tagged,
all_strictmodel whose fact was consolidated in an earlier round is never a candidate (its tag isn't in the final round'sconsolidated_tags) and stays stale forever. This matches the reported telemetry exactly: across a 36-round drain, rounds 1–35 enqueued zero refreshes; the final round enqueued refreshes only for the last documents ingested.Fix — accumulate across the chain, flush once at the end
Rather than refresh per-round (which, for a broad/global model during concurrent ingest, could re-refresh it once per round on partial data), the affected tags are accumulated across every round of the round-limited chain and the refresh is flushed a single time on the final round — so every affected model is refreshed exactly once, deduplicated but not dropped.
Mechanics:
pending_refresh_tagspayload field (mirrors howobservation_scopesalready flows). Non-final rounds defer refresh and carry the union forward; the final round flushes it via_trigger_mental_model_refreshes._submit_async_operation's dedupe-by-bank branch folds the incomingpending_refresh_tagsinto the surviving op's payload instead of silently dropping them. Safe under the existingFOR NO KEY UPDATEbank-row lock that already serialises submits for a bank.submit_async_refresh_mental_modelis still called withskip_if_in_flight=True, so two genuinely-parallel chains that both flush at their final rounds don't double-enqueue an overlapping model.Per-round tag scoping is unchanged (a round still only contributes the tags it actually consolidated); the change is that the union is preserved across the chain and flushed once.
Note on
updated_at:mark_consolidateddeliberately does not bumpupdated_at, and the staleness gate keys offupdated_at > last_refreshed_at— so a model whose memories were all ingested before the drain is not spuriously re-flagged.Tests
test_consolidation_multi_round_refresh_3411.pydrives a real multi-round consolidation chain through the worker executor (WorkerTaskBackend, so the re-queue lands as a pending op instead of recursing — matching production) with a small round limit, and asserts every entity-scoped model is refreshed exactly once as the backlog drains.test_requeue_dedupe_merges_pending_refresh_tagscovers the concurrency guard: a re-queue deduped into an already-pending consolidation must fold itspending_refresh_tagsinto the survivor's payload.Full suite green (19 passed): the two new tests plus
test_consolidation_round_limit,test_consolidation_reschedule_after_round,test_mental_model_consolidation_refresh_scope,test_mental_model_scheduled_refresh.test_consolidation_round_limit's re-queue assertion was updated to expect the newpending_refresh_tags=Nonekwarg (that bank is untagged → empty union).Crash durability (mid-round)
The accumulated union is also durable within a round. Each batch writes its succeeded source facts' tags into the op's
task_payloadinside that batch's own witness transaction — so the tags share the batch's fate (durable the instant the batch is). If a worker dies after a batch commits but before the round finishes, the op's retry skips those now-consolidated rows, but their tags are still intask_payload, so the final round refreshes their models anyway. The final round reads that durable value as the source of truth.SELECT ... FOR UPDATEserialises the concurrent batches of one op so their unions don't clobber each other.test_crash_mid_round_preserves_committed_batch_refresh_tagsinjects a crash right after the first batch commits, asserts its tag survived intask_payload, and that the retry refreshes every model — including the one consolidated pre-crash that the retry never reprocesses.Commits
fix(consolidation): refresh mental models every round …— first cut (per-round + in-flight dedup).refactor(consolidation): accumulate affected tags across rounds, refresh once at drain end— exactly-once design (accumulate + flush once + merge-on-dedupe).fix(consolidation): persist refresh tags per batch …— per-batch crash durability.