feat(memories): per-bank store capabilities - #3350
Merged
nicoloboschi merged 1 commit intoAug 10, 2026
Merged
Conversation
…l_for / owns_document_store_for) The two store capability flags (writes_memory_rows_in_sql, owns_document_store) were read as process-global class attributes, so one process could not host banks on different backends: a store that keeps some banks in SQL and others in a separate store has no single correct value. This adds per-bank forms and routes every BANK-SCOPED call site through them. - base.py: new writes_memory_rows_in_sql_for(bank_id) / owns_document_store_for( bank_id), defaulting to the class attribute — fully backward compatible, no existing store needs a change. - Converted the 34 bank-scoped call sites (memory_engine 20, consolidator 7, retain 6, reflect 1) to the per-bank form; bank_id is already in scope at each. - Left the 2 process-level gates in maintenance.py on the class attribute (they only decide whether the cross-store txn-recovery LOOP runs at all; the recovery itself is bank-scoped) — documented so they aren't "fixed" later. A store that routes banks overrides the _for methods; every existing single-store extension is unaffected. Tests (mock memory extension): per-bank capability defaults to the class attr; a RoutingMemories mock answers per bank. Full test_memories_extension.py green (152 passed), incl. the DB-backed engine-through-store tests that exercise the converted memory_engine sites end to end.
nicoloboschi
added a commit
that referenced
this pull request
Aug 11, 2026
list_banks consulted the per-bank capability with a bare bank_id that is not in scope inside the per-row loop (the row's id is row["bank_id"]). Since the argument is evaluated before the call, this raised NameError: name 'bank_id' is not defined for every org on the first bank — GET /banks 500'd outright — regardless of the store's capability. Introduced with per-bank store capabilities (#3350). Fix uses row["bank_id"]; adds a regression test covering the non-SQL store branch (fact_count sourced from the store's count_memories), which is where the bug lived.
nicoloboschi
added a commit
that referenced
this pull request
Aug 11, 2026
list_banks consulted the per-bank capability with a bare bank_id that is not in scope inside the per-row loop (the row's id is row["bank_id"]). Since the argument is evaluated before the call, this raised NameError: name 'bank_id' is not defined for every org on the first bank — GET /banks 500'd outright — regardless of the store's capability. Introduced with per-bank store capabilities (#3350). Fix uses row["bank_id"]; adds a regression test covering the non-SQL store branch (fact_count sourced from the store's count_memories), which is where the bug lived.
nicoloboschi
added a commit
that referenced
this pull request
Aug 11, 2026
list_banks consulted the per-bank capability with a bare bank_id that is not in scope inside the per-row loop (the row's id is row["bank_id"]). Since the argument is evaluated before the call, this raised NameError: name 'bank_id' is not defined for every org on the first bank — GET /banks 500'd outright — regardless of the store's capability. Introduced with per-bank store capabilities (#3350). Fix uses row["bank_id"]; adds a regression test covering the non-SQL store branch (fact_count sourced from the store's count_memories), which is where the bug lived.
nicoloboschi
added a commit
that referenced
this pull request
Aug 11, 2026
Re-applies the per-bank store-capability seam onto current main. The pluggable memories backend (#2917) is on main, but the per-bank capabilities landed later on feat/pluggable-memories-provider (#3350, plus fix #3381) while main advanced ~179 commits. A pluggable memories store may keep memory rows outside SQL and/or own the document store. The process-level flags writes_memory_rows_in_sql / owns_document_store gain per-bank forms — writes_memory_rows_in_sql_for(bank_id) and owns_document_store_for(bank_id), defaulting to the class attrs — and every bank-scoped call site in memory_engine, consolidation/consolidator, retain/* and reflect/tools consults the per-bank form. Process-level maintenance gates keep reading the class attr. Also two NameError fixes of the same class (a bare bank_id where the in-scope variable differs): list_banks (row["bank_id"], originally #3381) and get_chunk (chunk["bank_id"], newly surfaced by pyflakes while rebasing). Conflict resolution: only consolidation/consolidator.py conflicted — main added consolidation sites since the branch; all are bank_id-scoped, so all convert to the per-bank form. Validation: pyflakes on all changed engine files reports 0 undefined names; py_compile clean; per-bank + list_banks unit tests included.
nicoloboschi
added a commit
that referenced
this pull request
Aug 11, 2026
Re-applies the per-bank store-capability seam onto current main. The pluggable memories backend (#2917) is on main, but the per-bank capabilities landed later on feat/pluggable-memories-provider (#3350, plus fix #3381) while main advanced ~179 commits. A pluggable memories store may keep memory rows outside SQL and/or own the document store. The process-level flags writes_memory_rows_in_sql / owns_document_store gain per-bank forms — writes_memory_rows_in_sql_for(bank_id) and owns_document_store_for(bank_id), defaulting to the class attrs — and every bank-scoped call site in memory_engine, consolidation/consolidator, retain/* and reflect/tools consults the per-bank form. Process-level maintenance gates keep reading the class attr. Also two NameError fixes of the same class (a bare bank_id where the in-scope variable differs): list_banks (row["bank_id"], originally #3381) and get_chunk (chunk["bank_id"], newly surfaced by pyflakes while rebasing). Conflict resolution: only consolidation/consolidator.py conflicted — main added consolidation sites since the branch; all are bank_id-scoped, so all convert to the per-bank form. Validation: pyflakes on all changed engine files reports 0 undefined names; py_compile clean; per-bank + list_banks unit tests included.
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.
Why
The two memories-store capability flags —
writes_memory_rows_in_sqlandowns_document_store— are read as process-global class attributes, so one process cannot host banks on different backends. A store that keeps some banks in SQL and routes others to a separate store (per-org routing) has no single correct value: reportTrueand the separate-store banks silently write to SQL; reportFalseand the SQL banks' inline-SQL no-op methods lose writes.This adds per-bank forms of both flags and routes every bank-scoped call site through them, so a store can answer per bank. It is the foundation for a routing memories extension (some orgs on Postgres, some on a separate store) without the OSS engine knowing anything about the specific backend.
What
base.py: newwrites_memory_rows_in_sql_for(bank_id)/owns_document_store_for(bank_id), defaulting to the class attribute → fully backward compatible; no existing store changes.bank_idis already in scope at each.maintenance.pyon the class attribute — they only decide whether the cross-store txn-recovery loop runs at all; the recovery itself is bank-scoped. Documented so they aren't "fixed" later.A single-store extension is unaffected (defaults delegate to the class attr). A routing store overrides the two
_formethods.Tested (mock memory extension)
test_per_bank_capability_defaults_to_the_class_attribute— the_formethods return the class attr forPostgresMemoriesandInMemoryMemories.test_a_routing_store_answers_capabilities_per_bank— aRoutingMemoriesmock reports SQL for one bank and separate-store for another, with the process-level class attr kept False so recovery still runs.test_memories_extension.pygreen (152 passed), including the DB-backed engine-through-store tests that drive real ops through the convertedmemory_enginesites.