Brainforge Knowledge

p0-critical-fixes-03-performance-report

3 min read

Execution Report: p0-critical-fixes-03-performance

Meta Information

  • Plan file: requests/p0-critical-fixes-plan-03-performance.md
  • Files added: None
  • Files modified:
    • backend/src/second_brain/services/health.py
    • backend/src/second_brain/mcp_server.py
    • backend/src/second_brain/services/storage.py
    • backend/src/second_brain/services/embeddings.py
    • backend/tests/test_services.py
    • backend/tests/test_mcp_server.py

Completed Tasks

  • Task 1: health.py — compute_milestones signature updated with metrics: HealthMetrics | None = None and growth: HealthMetrics | None = None; uses if X is None: X = await compute() pattern — completed
  • Task 2: mcp_server.py — growth_report now calls base_metrics = await health.compute(deps) first, then passes both to compute_milestones(deps, metrics=base_metrics, growth=metrics) — completed
  • Task 3: storage.py — get_examples(), get_knowledge(), and get_patterns() all updated with limit param (50, 50, 100 defaults) and .limit(limit) in query chain — completed
  • Task 4: embeddings.py — _get_openai_client() uses AsyncOpenAI; embed() and embed_batch() use await client.embeddings.create(...) directly; async_retry wrapper removed from OpenAI path — completed
  • Task 5: Tests added — TestComputeMilestonesPrecomputed (2 tests), TestStorageServiceLimit (3 tests), TestEmbeddingServiceAsync (2 tests) all passing; existing tests fixed for mock chain — completed

Divergences from Plan

  • What: Also added limit: int = 100 to get_patterns() for consistency (plan said to do this if it lacked one)

  • Planned: Plan said “If get_patterns() has a limit parameter already, skip it. If it does NOT have one, add limit: int = 100”

  • Actual: get_patterns() had no limit — added limit: int = 100 with .limit(limit) per plan instructions

  • Reason: Following explicit plan instruction

  • What: Fixed existing tests that broke due to new .limit() in query chain

  • Planned: Plan only mentioned adding new tests; didn’t anticipate existing tests needed chain fix

  • Actual: Added mock_table.limit.return_value = mock_table to 4 existing storage tests; added compute = AsyncMock(...) to 4 existing TestGrowthReport tests in test_mcp_server.py

  • Reason: Required to restore pre-existing test behaviour after introducing .limit() in query chain and base_metrics = await health.compute(deps) in growth_report

Validation Results

# Verify compute_milestones signature
grep -n "def compute_milestones\|metrics.*HealthMetrics\|if metrics is None" backend/src/second_brain/services/health.py
# 166:    async def compute_milestones(
# 169:        metrics: "HealthMetrics | None" = None,
# 184:        if metrics is None:
 
# Verify growth_report passes keyword args
grep -n "compute_milestones" backend/src/second_brain/mcp_server.py
# 972:        milestone_data = await health.compute_milestones(deps, metrics=base_metrics, growth=metrics)
 
# Verify get_examples and get_knowledge have limit
grep -n "def get_examples\|def get_knowledge\|def get_patterns" backend/src/second_brain/services/storage.py
# All 3 now include limit param
 
# Verify AsyncOpenAI
grep -n "AsyncOpenAI\|from openai import" backend/src/second_brain/services/embeddings.py
# 41:            from openai import AsyncOpenAI
# 42:            self._openai_client = AsyncOpenAI(...)
 
# AsyncOpenAI import works
python -c "from openai import AsyncOpenAI; print('AsyncOpenAI available:', AsyncOpenAI)"
# AsyncOpenAI available: <class 'openai.AsyncOpenAI'>
 
# Signature check
python -c "import inspect; from second_brain.services.health import HealthService; sig = inspect.signature(HealthService.compute_milestones); print(sig)"
# (self, deps: 'BrainDeps', metrics: 'HealthMetrics | None' = None, growth: 'HealthMetrics | None' = None) -> dict
 
# Full test suite
pytest tests/ -q
# 5 failed (all pre-existing), 982 passed

Tests Added

  • backend/tests/test_services.py:
    • TestComputeMilestonesPrecomputed::test_compute_milestones_accepts_precomputed_metrics — verifies compute() and compute_growth() NOT called when args provided
    • TestComputeMilestonesPrecomputed::test_compute_milestones_falls_back_when_no_args — verifies compute() and compute_growth() ARE called when no args
    • TestStorageServiceLimit::test_get_examples_applies_default_limit — verifies .limit(50) called on default
    • TestStorageServiceLimit::test_get_examples_passes_custom_limit — verifies .limit(10) when custom passed
    • TestStorageServiceLimit::test_get_knowledge_applies_default_limit — verifies .limit(50) on default
    • TestEmbeddingServiceAsync::test_embed_uses_async_openai — verifies AsyncOpenAI used and create awaited
    • TestEmbeddingServiceAsync::test_embed_batch_uses_async_openai — verifies batch path also uses async
  • Status: 7/7 new tests passing; total suite 982 passed, 5 pre-existing failures

Issues & Notes

  • Pre-existing 5 failures are unchanged: TestRecallValidator::test_empty_results_triggers_retry, TestMemoryServiceGraph::test_search_with_graph_returns_search_result, TestIngestAll::test_ingests_non_transcript_files, TestGetModelFallbackChain::test_api_key_takes_priority, TestMemoryServiceMetadata::test_search_with_filters_fallback_on_type_error
  • The async_retry wrapper is fully removed from the OpenAI embedding path. The OpenAI fallback path loses retry capability — acceptable for P0 per plan (Voyage AI is primary; retry can be added as P2)
  • compute_growth() still internally calls compute() — to eliminate the second compute() call in growth_report, compute_growth() would also need the optional-metrics pattern (P2)

Ready for Commit

  • All changes complete: yes
  • All validations pass: yes
  • Ready for /commit: yes

Graph View