fix(memory): don't take a write lock to backfill nothing on every open (#368) - #980
Open
ziomik wants to merge 1 commit into
Open
fix(memory): don't take a write lock to backfill nothing on every open (#368)#980ziomik wants to merge 1 commit into
ziomik wants to merge 1 commit into
Conversation
bradbrok#368) _migrate_backfill_review_schedule ran its UPDATE unconditionally on every ReflectionStore open, so every component that constructs a store took a write transaction at daemon start even when no row needed scheduling — which is the steady state. Guard it with a read-only EXISTS probe and write only when there is something to schedule. Note this is deliberately NOT turned into a one-shot migration despite its name and old docstring: insert() never sets next_review_date, so this UPDATE is also what makes newly created memories reviewable. Running it once would silently stop the review system from picking up anything inserted later. The docstring now says so, and a test pins the behaviour. The row predicate moves into _NEEDS_REVIEW_DATE, shared by the guard and the UPDATE so they cannot drift: a narrower guard would skip real work, a wider one would write when there is nothing to do.
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
1 participant
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 #368.
Il problema
_migrate_backfill_review_scheduleeseguiva la suaUPDATEin modo incondizionato a ogni apertura diReflectionStore. Diversi componenti ne costruiscono uno all'avvio del daemon, quindi ognuno prendeva una transazione di scrittura anche quando nessuna riga aveva bisogno di essere schedulata — che e' lo stato normale a regime.Perche' NON e' stata resa una migrazione one-shot
Il nome del metodo e il vecchio docstring ("on first run") dicono migrazione una-tantum, ed e' la lettura che porterebbe naturalmente a un marker di versione dello schema. Sarebbe stato un errore silenzioso.
insert()non imposta mainext_review_date— le nuove righe restanoNULL. QuestaUPDATEe' quindi anche cio' che rende recensibile una memoria appena creata. Eseguirla una volta sola avrebbe fatto smettere al sistema di review di raccogliere qualunque cosa inserita in seguito, senza alcun errore visibile.Quindi il comportamento resta identico: gira a ogni apertura. Cambia solo che non prende piu' il lock quando non c'e' niente da fare.
Il fix
Una sonda read-only (
SELECT 1 ... LIMIT 1) precede laUPDATE; si scrive solo se esiste davvero almeno una riga da schedulare.Il predicato delle righe e' estratto in
_NEEDS_REVIEW_DATE, condiviso da guardia eUPDATEcosi' i due non possono divergere: una guardia piu' stretta salterebbe lavoro reale, una piu' larga rimetterebbe la scrittura a vuoto che questa PR elimina.Il docstring e' stato riscritto per dire cosa fa davvero il metodo, visto che quello vecchio era esattamente la premessa fuorviante.
Test
Nuovo file
tests/test_memory_backfill_review_writes.py(4 test):salience >= 4) restanoNULLe non entrano nel ciclo di reviewVerifica eseguita:
UPDATEparte anche su un DB vuoto), 2 passed — i 2 verdi sono le guardie di non-regressione, il loro compito e' passare da subitopinky_memory(test_memory_store,test_memory_server,test_kg_*,test_memory_cross_agent,test_memory_heal_unembedded,test_shared_mcp)ruff checkpulito su entrambi i fileNote
Branch basato su
origin/main, come #979. Le due PR toccano hunk diversi dello stesso file e non hanno dipendenze d'ordine: si possono mergiare in qualunque sequenza.🤖 Opened by Engineer