fix(mental-models): keep at most one queued refresh per model (#3487) - #3550
Merged
Conversation
A bank whose refresh queue drains slower than it fills accumulated one refresh_mental_model operation per model per consolidation round — 12k pending operations covering 259 models, ~45 identical copies each, every copy a full recall + LLM refresh when it eventually ran. The in-flight guard existed but was opt-in per call site, so any enqueue path that did not ask for it (and every path before #3411) piled up copies. Make the floor structural instead: a submit for a model that already has a *queued* refresh always folds into it and returns that operation's id. Nothing is lost — a refresh carries no per-request options and the queued one has not started, so it still reads whatever the caller just changed. skip_if_in_flight now only widens the guard to an already-*running* refresh, which an explicit refresh must not fold into: it may have read the model before the caller's edit. The check moves out of the INSERT and back in front of it, where the bank-row FOR NO KEY UPDATE lock (held for the rest of the transaction) already serialises submits for the bank and makes check-and-insert atomic. That also makes it work on Oracle: the previous INSERT ... SELECT ... WHERE NOT EXISTS is a FROM-less SELECT there, and its bind-parameter JSON key was never rewritten to JSON_VALUE, so since #3411 every after-consolidation refresh submit raised on Oracle and was swallowed as a warning.
…test (#3487) The eight-way concurrent submit test passed with the bank-row lock removed — asyncio happened to run each short transaction to completion before the next, so it never actually raced. Stall every in-flight lookup before it returns, so all eight submits would sit between their lookup and their INSERT at once. With the lock it still queues one operation; without it the same test inserts eight.
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 #3487
What was wrong
A bank whose refresh queue drains slower than it fills accumulated one
refresh_mental_modeloperation per model per consolidation round — the report saw 12,461 pending operations covering 259 models (~45 identical copies each), each copy a full recall + LLM refresh when it eventually ran.The dominant source is already fixed on main: the after-consolidation flush only started asking for the in-flight guard in #3411 (Aug 12), two days before the issue was filed, so the reporting deployment predates it. A repro on current main confirms 5 consolidation flushes queue 1 operation, not 5.
Two holes remained:
skip_if_in_flight=True, and the one that forgot is exactly what produced the pile. Nothing structurally enforced "at most one pending refresh per(bank_id, mental_model_id)".INSERT ... SELECT ... WHERE NOT EXISTS— a FROM-less SELECT there — and carried the JSON key as a bind parameter, which the rewriter leaves untouched (only a literal key becomesJSON_VALUE). So since Mental models with refresh_after_consolidation are not refreshed when consolidation spans multiple rounds #3411 every after-consolidation refresh submit raised on Oracle and was swallowed by the per-modelexcept Exceptionas a warning: those models silently stopped auto-refreshing.The fix
deduplicated=True— for every caller, not just the ones that opt in. Nothing is lost: a refresh carries no per-request options, and a queued operation has not started, so it still reads whatever the caller just edited.skip_if_in_flightnow only widens the guard to an already-running refresh. An explicit user refresh still gets its own operation in that case, since the running one may have read the model before their edit — which is what the previous behaviour was actually protecting.FOR NO KEY UPDATElock that already serialises submits for the bank (the same patterndedupe_by_bankuses). It stays race-proof under concurrent flushes, and it is now valid on both dialects: the key is inlined (rejected unless a plain identifier) so Oracle rewrites it toJSON_VALUE, and the insert stays aVALUESform.Not done: the partial unique index the issue suggests would also reject the deliberate second operation queued behind a running refresh.
Tests
New
test_mental_model_refresh_pending_dedupe_3487.py:All four fail without the fix.
test_user_triggered_refresh_is_not_deduplicatedbecomes..._still_queues_while_one_is_processingfor the new contract.Green locally: the 13 dedupe/cron tests, 56 mental-model + consolidation tests, 379 mental-model/knowledge/template/operation/MCP tests, 140 HTTP-integration tests, and
./scripts/hooks/lint.sh.Docs
The mental-models API page now says refreshes coalesce and what that means for the returned
operation_id(docs skill regenerated).