feat(server): add AppContext.ensure_initialized plumbing (#399 phase 1)#400
Merged
feat(server): add AppContext.ensure_initialized plumbing (#399 phase 1)#400
Conversation
Plumbing-only refactor for the lazy DB-init work in #399. Behavior unchanged: app_lifespan still calls ensure_initialized eagerly, so the DB is created at the same point in startup as before. - Hide storage/embedder/index_engine/search_pipeline/embedding_broken/ llm_provider behind read-only properties backed by AppContext._components - Add async ensure_initialized() with _init_lock so the future per-tool-call init path won't race - Add AppContext.from_components() classmethod for bootstraps that already have a Components (mm watchdog CLI, degraded-mode test fixture) - Add async close() for symmetric teardown - Drop the watcher field from AppContext (handler usage was zero; lifespan keeps a local var) - dedup_scanner is now derived from Components.storage + .embedder inside ensure_initialized / from_components rather than constructed by the lifespan Phase 2 will migrate the ~33 handler files to call ensure_initialized; Phase 3 will drop the eager call so the DB stays absent until first tool call. Co-Authored-By: Claude <[email protected]>
Three MAJORs + two MINORs from the review on PR #400: MAJOR: ensure_initialized now tears down create_components's output if a post-factory step raises. Previously a DedupScanner(storage=..., ...) failure would leave the already-opened sqlite handle + embedder session orphaned — no path ever called close_components for them. Explicit try/except around the post-factory chain closes the leak. MAJOR: property access before ensure_initialized now raises RuntimeError via a _require_initialized helper instead of `assert ... is not None`. asserts are stripped under `python -O` / PYTHONOPTIMIZE=1, at which point pre-init access would hit a synthesized AttributeError with no hint about the real cause (ensure_initialized not called). RuntimeError survives the optimizer and carries an actionable message. MAJOR: from_components vs ensure_initialized now track ownership via a private _owns_components flag. close() only calls close_components when _owns_components is True — i.e. only on the path that actually built the Components. from_components callers (cli_components context manager, test fixtures) retain ownership; close() used to double-close their handles. The flag also resets on close() so post-close property access still fails via _require_initialized. MINOR: health_watchdog writer side now goes through a typed set_health_watchdog method instead of lifespan poking ctx._health_watchdog directly. Read side unchanged (the health_watchdog property). MINOR: drop the `**kwargs: Any` escape hatch from `from_components` — there were no real callers using it; collapsing to the bare components argument tightens the public surface. Tests (13 total, +4 new + 1 modernized): - test_ensure_initialized_closes_components_if_post_factory_step_raises: locks in the orphan-cleanup fix via patched close_components + a DedupScanner that blows up. - test_close_after_from_components_does_not_touch_caller_owned: verifies close_components is NOT invoked on the from_components path, and that the context still drops its component view so stale access fails loudly. - test_close_after_ensure_initialized_closes_components: inverse — the path that DID own Components does close them. - test_set_health_watchdog_exposes_via_property: locks in the lifespan-writer / property-reader seam. - test_storage_access_before_init_raises: switched AssertionError → RuntimeError expectation. Co-Authored-By: Claude <[email protected]>
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
Phase 1 of #399 —
AppContextplumbing for the upcoming lazy DB-init refactor.Summary
storage/embedder/index_engine/search_pipeline/embedding_broken/llm_providerfrom directAppContextfields to read-only properties backed by an internal_componentsslot.async ensure_initialized()with_init_lockfor concurrent-first-call serialization.AppContext.from_components(...)classmethod for CLI / test bootstraps that already have aComponents(e.g.mm watchdog, degraded-mode test fixture).async close()for symmetric teardown.watcherfield fromAppContext— handler usage was zero; the lifespan keeps a local var.dedup_scanneris now derived fromComponents.storage+.embedderinsideensure_initialized/from_componentsrather than passed in by the lifespan.app_lifespanstill callsawait ctx.ensure_initialized()eagerly, so behavior is unchanged. Phase 2 (handler migration) and Phase 3 (drop the eager call → DB stays absent until first tool call) are stacked PRs.Why split it
#399 traces the
~/.memtomem/memtomem.dbside-effect on bare MCP handshake toapp_lifespancallingcreate_componentsbefore yielding. The fix needs three things to land safely:Splitting it keeps reviewers focused on the lock semantics + property migration here, rather than reading 30+ mechanical handler diffs at the same time.
Test plan
test_server_app_context.py(9 tests): concurrent-first-call → single factory invocation, idempotent re-call, failure releases lock for retry,from_componentsshort-circuits the factory, pre-initstorageaccess raises, pre-initembedding_broken/llm_provider/dedup_scanner/health_watchdogreturnNone(mirrors the old field defaults).test_server_degraded_mode.py(4 tests): fixture switched toAppContext.from_components.test_health_watchdog,test_trust_ux,test_tools_logic,test_server_helpers) — 209 tests pass unchanged; they go through mockAppContextsurrogates.uv run pytest packages/memtomem/tests -m "not ollama"→ 2144 passed.uv run ruff check packages/memtomem/src+uv run ruff format --checkclean.Related
_get_app_initialized/ inlineawait app.ensure_initialized().ensure_initializedfromapp_lifespan+ move watcher / scheduler / watchdog start intoensure_initialized. That's where the user-visible behavior change (DB stays absent on bare handshake) actually lands.🤖 Generated with Claude Code