fix(memory): restore beam initialization lifecycle#470
Conversation
📝 WalkthroughWalkthroughMnemosyne now tracks shared per-thread database ownership, initializes BeamMemory during construction, and conditionally closes shared resources only for the final owner. Tests cover emitter wiring, idempotent cleanup, shared connections, and constructor failure recovery. ChangesShared connection lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_beam.py`:
- Around line 955-965: Extend test_constructor_wires_beam_before_close to assert
that the constructed BeamMemory retains Mnemosyne._stream_emit as its
event-emitter callback, using the appropriate BeamMemory callback attribute.
Keep the existing construction, idempotent close, and successor lifecycle
assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fe1675fa-1e0e-4287-a4e2-b1a47c98adac
📒 Files selected for processing (2)
mnemosyne/core/memory.pytests/test_beam.py
c991481 to
3037260
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mnemosyne/core/memory.py`:
- Around line 209-214: Initialize Mnemosyne._closed before constructing
BeamMemory, then wrap the BeamMemory construction in an exception path that
invokes close() or the established cleanup routine before re-raising the
original exception. Ensure failed construction cannot reach __del__ with missing
lifecycle state or leave opened connections, while preserving normal
initialization behavior.
- Around line 209-214: Update Mnemosyne’s BeamMemory lifecycle around the
constructor wiring and close() so shared thread-local BeamMemory handles are not
closed while another live instance uses them. Implement per-instance
ownership/refcounting or enforce a single live owner, ensuring each Mnemosyne
retains a valid self.beam.conn until its own cleanup is complete.
In `@tests/test_beam.py`:
- Around line 955-967: Strengthen test_constructor_wires_beam_before_close by
spying on mem.stream.emit, enabling streaming with mem.enable_streaming(),
invoking mem.beam._event_emitter with a sentinel event, and asserting the stream
receives that event. If lifecycle ownership requires it, also cover closing two
live Mnemosyne instances independently while preserving the existing
idempotent-close assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f22eb3cb-82bd-445a-adce-54627f803348
📒 Files selected for processing (2)
mnemosyne/core/memory.pytests/test_beam.py
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@mnemosyne/core/memory.py`:
- Around line 30-31: Update mnemosyne/core/memory.py lines 30-31 to store
thread-local connections per database path, then adjust the acquisition and
release logic in the relevant memory connection functions (lines 82-136 and
240-286) to retain, resolve, and close the handle for the released owner key.
Update mnemosyne/core/beam.py lines 490-507 to replace the single cached BEAM
connection with path-addressable storage, preserving correct cache invalidation
and lifecycle cleanup for both BEAM and core memory.
In `@tests/test_beam.py`:
- Around line 976-999: Expand the lifecycle tests around
test_close_keeps_shared_beam_connection_for_live_instance and
test_constructor_failure_releases_provisional_connection to assert both
second.conn and second.beam.conn remain usable after first.close(). After
recovery and final close, verify the failed owner’s connection/registry key is
removed. Add coverage with two live Mnemosyne instances using different database
paths, asserting each SQLite and BEAM handle closes when its final owner is
released.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 54409b7f-a014-411b-b74a-3a79682c4b14
📒 Files selected for processing (3)
mnemosyne/core/beam.pymnemosyne/core/memory.pytests/test_beam.py
| _instance_connection_lock = threading.Lock() | ||
| _instance_connection_refcounts: Dict[tuple, int] = {} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Align per-database ownership with per-database connection storage.
The refcounts distinguish databases, but each module retains only one thread-local connection. Switching databases displaces the prior handles, and final-owner cleanup cannot find them.
mnemosyne/core/memory.py#L30-L31: store connection ownership alongside actual per-path handles.mnemosyne/core/memory.py#L82-L136: resolve and close the handle associated with the released owner key.mnemosyne/core/memory.py#L240-L286: retain per-instance/per-path handles through acquisition and final release.mnemosyne/core/beam.py#L490-L507: replace the single cache slot with path-addressable BEAM connections.
As per path instructions, verify cache invalidation correctness and lifecycle cleanup for BEAM and core memory.
📍 Affects 2 files
mnemosyne/core/memory.py#L30-L31(this comment)mnemosyne/core/memory.py#L82-L136mnemosyne/core/memory.py#L240-L286mnemosyne/core/beam.py#L490-L507
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@mnemosyne/core/memory.py` around lines 30 - 31, Update
mnemosyne/core/memory.py lines 30-31 to store thread-local connections per
database path, then adjust the acquisition and release logic in the relevant
memory connection functions (lines 82-136 and 240-286) to retain, resolve, and
close the handle for the released owner key. Update mnemosyne/core/beam.py lines
490-507 to replace the single cached BEAM connection with path-addressable
storage, preserving correct cache invalidation and lifecycle cleanup for both
BEAM and core memory.
Source: Path instructions
| def test_close_keeps_shared_beam_connection_for_live_instance(self, temp_db): | ||
| first = Mnemosyne(session_id="first", db_path=temp_db) | ||
| second = Mnemosyne(session_id="second", db_path=temp_db) | ||
|
|
||
| first.close() | ||
| assert second.beam.conn.execute("SELECT 1").fetchone()[0] == 1 | ||
| second.close() | ||
|
|
||
| def test_constructor_failure_releases_provisional_connection(self, temp_db, monkeypatch): | ||
| import mnemosyne.core.memory as memory_module | ||
|
|
||
| real_beam = memory_module.BeamMemory | ||
|
|
||
| def fail_beam(*args, **kwargs): | ||
| raise RuntimeError("beam construction failed") | ||
|
|
||
| monkeypatch.setattr(memory_module, "BeamMemory", fail_beam) | ||
| with pytest.raises(RuntimeError, match="beam construction failed"): | ||
| Mnemosyne(session_id="broken", db_path=temp_db) | ||
|
|
||
| monkeypatch.setattr(memory_module, "BeamMemory", real_beam) | ||
| recovered = Mnemosyne(session_id="recovered", db_path=temp_db) | ||
| assert isinstance(recovered.beam, BeamMemory) | ||
| recovered.close() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the lifecycle tests prove both resources are released.
The shared-instance test checks only beam.conn, while the constructor-failure test still passes if the failed owner leaks a refcount. Extend this group to:
- verify
second.connremains usable; - assert the failed owner key is removed after recovery and close;
- exercise two live instances using different database paths and confirm each displaced SQLite/BEAM handle is closed at its final-owner release.
As per path instructions, tests require comprehensive coverage and meaningful assertions rather than merely successful reconstruction.
🧰 Tools
🪛 Ruff (0.15.21)
[warning] 989-989: Missing return type annotation for private function fail_beam
Add return type annotation: NoReturn
(ANN202)
[warning] 989-989: Missing type annotation for *args
(ANN002)
[warning] 989-989: Unused function argument: args
(ARG001)
[warning] 989-989: Missing type annotation for **kwargs
(ANN003)
[warning] 989-989: Unused function argument: kwargs
(ARG001)
[warning] 990-990: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_beam.py` around lines 976 - 999, Expand the lifecycle tests around
test_close_keeps_shared_beam_connection_for_live_instance and
test_constructor_failure_releases_provisional_connection to assert both
second.conn and second.beam.conn remain usable after first.close(). After
recovery and final close, verify the failed owner’s connection/registry key is
removed. Add coverage with two live Mnemosyne instances using different database
paths, asserting each SQLite and BEAM handle closes when its final owner is
released.
Source: Path instructions
1 participant
Summary
Restores the pre-#382 Phase 8 lazy-state and
BeamMemoryinitialization toMnemosyne.__init__. The WAL cleanup methods remain lifecycle-only.The #382 insertion accidentally left this initialization under
__del__, so normal instances had nobeamattribute and the main CI failed.Verification
TestMnemosyneIntegration::test_legacy_and_beam_dual_writeraisedAttributeError: Mnemosyne has no attribute beam.44 passed.py_compileandgit diff --checkpass.Note: the unrelated current-main polyphonic recall failure (
PolyphonicResulthas nocontent) remains outside this narrow regression fix.Summary
This PR corrects
Mnemosyne’s core memory lifecycle by restoring Phase 8 lazy-subsystem placeholder setup andBeamMemoryconstruction back intoMnemosyne.__init__(where it belongs), undoing an accidental relocation under__del__introduced previously. That ensuresBeamMemoryis created during construction with the intended event wiring (event_emitter=self._stream_emit), whileclose()remains focused on WAL-cleanup and idempotent shutdown. It also adds constructor-and-close regression coverage, including safe behavior across multiple wrappers and exception paths.BeamMemorywith the correct emitter. Removes duplicated/late Phase 8 initialization logic, preventing partially-initialized BEAM state from reaching runtime.Mnemosynewrappers in the same thread (and/or across different banks/db paths) don’t double-close shared connections._close_connection/_close_beam_connectionnow support targeteddb_path-aware cleanup to avoid invalidating a newer live instance that switched banks.close()is invoked to release owners), and reinforcingclose()idempotency. Regression tests explicitly validate constructor wiring and safe repeated/second-instance shutdown behavior.