Skip to content

revert: unsafe #382 connection lifecycle change#477

Merged
AxDSan merged 1 commit into
mnemosyne-oss:mainfrom
dplush:revert/382-unsafe-lifecycle
Jul 16, 2026
Merged

revert: unsafe #382 connection lifecycle change#477
AxDSan merged 1 commit into
mnemosyne-oss:mainfrom
dplush:revert/382-unsafe-lifecycle

Conversation

@dplush

@dplush dplush commented Jul 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Reverts c527c40 (#382). The change accidentally moved Mnemosyne constructor initialization under __del__, breaking every normal instance with a missing beam attribute. Follow-up attempts exposed that safe multi-DB lifecycle support needs a defined lease contract, not incremental patching.

This restores the previously stable connection behavior. The WAL cleanup design will be revisited separately with an explicit lifecycle contract and test matrix.

Verification

Supersedes #470 and #472.

Summary

Reverts the lifecycle changes that caused normal Mnemosyne instances to lose their beam attribute, restoring stable core initialization and connection behavior.

Architectural impact

  • Restores the existing working, episodic, and BEAM memory architecture without changing tiering, retrieval, consolidation, veracity, or synchronization behavior.
  • Removes the experimental thread-local connection shutdown and WAL checkpointing paths from BEAM and Mnemosyne.
  • Preserves local-first operation by avoiding new remote dependencies or changes to local persistence.
  • Keeps Hermes, MCP, and CLI integration surfaces behaviorally unchanged while restoring their access to properly initialized Mnemosyne instances.
  • Simplifies the lifecycle implementation and avoids an implicit destructor-based contract; explicit multi-database lifecycle handling and WAL cleanup should be addressed separately with defined semantics and test coverage.

Validation

  • 43 Beam, CLI, and allowlist tests pass.
  • py_compile succeeds.
  • git diff --check succeeds.

@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 805a1915-1ba0-4e68-bd80-95e6cdf55f75

📥 Commits

Reviewing files that changed from the base of the PR and between 0a64d2e and f86f125.

📒 Files selected for processing (2)
  • mnemosyne/core/beam.py
  • mnemosyne/core/memory.py
💤 Files with no reviewable changes (2)
  • mnemosyne/core/memory.py
  • mnemosyne/core/beam.py

📝 Walkthrough

Walkthrough

The pull request removes thread-local SQLite cleanup, WAL checkpointing, BEAM connection cleanup, and the corresponding Mnemosyne.close() and destructor methods.

Changes

Connection cleanup removal

Layer / File(s) Summary
Remove connection cleanup helpers
mnemosyne/core/memory.py, mnemosyne/core/beam.py
Removes the SQLite cleanup, manual WAL checkpoint, and BEAM connection cleanup helpers.
Remove Mnemosyne shutdown methods
mnemosyne/core/memory.py
Removes Mnemosyne.close() and Mnemosyne.__del__() methods.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes that this PR reverts the unsafe connection lifecycle change from #382.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@dplush

dplush commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

Verification note: this revert removes the Mnemosyne has no attribute beam failure (0 occurrences in the PR CI logs). The remaining Python failures are only the separate #389 PolyphonicResult.content regression. Local focused Beam/CLI/allowlist coverage: 43 passed; CodeRabbit found no actionable issues.

@dplush

dplush commented Jul 16, 2026

Copy link
Copy Markdown
Contributor Author

@AxDSan
Context on the PR sequence:

The intent is to stabilize main first, then revisit WAL/connection cleanup as a separately designed change with clear ownership semantics and a full lifecycle test matrix.

Here is the request for the following procedure:

  1. First, please merge revert: unsafe #382 connection lifecycle change #477

  2. Immediately afterward, I’ll rebase fix(polyphonic): hydrate content before diversity ranking #471 onto the new “main”

  3. After that, I won’t touch Lifecycle/WAL again right away

    • only as a standalone, design-first topic with a lease contract and tests.

@AxDSan
AxDSan merged commit b366a19 into mnemosyne-oss:main Jul 16, 2026
4 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants