fix(memory): use shared project-wide memory for cross-spec learning (#905)
* fix(memory): use shared project-wide memory for cross-spec learning Task execution memory was isolated per spec (GroupIdMode.SPEC), preventing learnings from one spec from benefiting other specs. Changed all GraphitiMemory instantiations to use GroupIdMode.PROJECT for shared project-wide context. This aligns task memory with PR review memory, which already uses shared databases for cross-session learning. Fixes #205 * refactor(memory): centralize GraphitiMemory instantiation via helper Use the existing get_graphiti_memory helper function instead of directly instantiating GraphitiMemory in multiple places. This reduces code duplication and improves maintainability by having a single source of truth for memory creation. The helper already uses GroupIdMode.PROJECT for cross-spec learning, so this refactor maintains the original fix while centralizing the logic. Addresses review comments on #905 * refactor(memory): improve error handling for GraphitiMemory instantiation Move get_graphiti_memory() calls inside try/except blocks to properly catch construction errors. Also use explicit 'is None' checks instead of truthiness to avoid surprising __bool__ behavior. - In get_graphiti_context: Move memory creation inside try block - In save_session_memory: Move memory creation inside try block - In _save_to_graphiti_async: Remove redundant is_graphiti_enabled check, use 'is None' for explicit None checking These changes ensure that any construction errors are caught and handled, allowing graceful fallback to file-based memory. Addresses additional review comments on #905 * style(memory): use explicit None checks in finally blocks Replace truthy checks with explicit 'is not None' checks in finally blocks for consistency with other explicit None checks in memory_manager.py. Addresses review comment on #905 * fix(memory): use explicit None check in second finally block Update the finally block in save_session_memory to use explicit None check for consistency. The first finally block was updated but this one was missed. Addresses remaining review comment on #905 * refactor(memory): use finally block for consistent cleanup in save_to_graphiti_async Refactor save_to_graphiti_async to use a finally block with explicit None check for resource cleanup, matching the pattern established in memory_manager.py. Previously, close() was called in both the try and except blocks, which could lead to resource leaks in certain edge cases. The finally block ensures cleanup always occurs. Addresses CodeRabbit review feedback on #905 * refactor(memory): add type hints and improve cleanup safety - Add return type annotation to get_graphiti_memory() using TYPE_CHECKING to avoid circular imports while keeping call sites honest - Wrap memory.close() in try/except within finally blocks to prevent close() exceptions from overriding success/failure flows - Use explicit 'memory is not None' checks to avoid misleading warnings when memory isn't available - Distinguish between 'memory is None' (not available) and 'memory.is_enabled is False' (disabled) for clearer logging Addresses CodeRabbit review feedback on #905 * fix(memory): wrap close() in try/except in save_to_graphiti_async finally Wrap graphiti.close() in try/except within the finally block to prevent close() exceptions from overriding successful outcomes. This matches the pattern established in memory_manager.py for consistent resource cleanup. Addresses CodeRabbit review feedback on #905 * fix(memory): address follow-up review findings - Wrap close() in try/except in tools/memory.py finally block to match the pattern in graphiti_helpers.py and memory_manager.py, preventing close() exceptions from overriding successful outcomes - Update get_graphiti_memory() default in integrations/graphiti/memory.py from GroupIdMode.SPEC to GroupIdMode.PROJECT for consistency with the PR's intent of enabling cross-spec learning by default - Add deprecation note explaining the default change Addresses follow-up review findings on #905: - NEW-001: Inconsistent close() handling - e87df0a37e94: Duplicate get_graphiti_memory function with different default * refactor(memory): log close failures at debug level Update all finally blocks to log memory.close() failures at debug level instead of silently swallowing them. This provides useful diagnostics while still preventing close() exceptions from overriding the main result. Changes: - tools/memory.py: Add logger.debug() with exc_info for close failures - graphiti_helpers.py: Add logger.debug() with exc_info for close failures - memory_manager.py: Add logger.debug() with exc_info for close failures (2 locations) The debug-level logging ensures close failures are visible for debugging but do not affect the primary operation outcome. Addresses review feedback on #905 * fix(memory): log close failures in nested finally block Add debug logging to the nested finally block in save_session_memory that was missed by the previous replace_all operation due to different indentation levels. This ensures all close failures are logged at debug level for debugging purposes while still preventing them from overriding the main result. Addresses review feedback on #905 * fix(logging): use exc_info=True for proper traceback in debug logs Change logger.debug calls to use exc_info=True instead of exc_info=e when logging close failures. This ensures the current exception's traceback is included in the log output for better debugging. exc_info=e only includes the exception object but not the traceback, while exc_info=True captures the full traceback information from the current exception context. Changes: - memory_manager.py: Update both finally blocks (2 locations) - graphiti_helpers.py: Update finally block - tools/memory.py: Update finally block Addresses review feedback on #905 --------- Co-authored-by: StillKnotKnown <stillknotknown@users.noreply.github.com> Co-authored-by: Andy <119136210+AndyMik90@users.noreply.github.com>
This commit is contained in:
StillKnotKnown
@@ -24,6 +24,7 @@ from graphiti_config import get_graphiti_status, is_graphiti_enabled
|
||||
# Import from parent memory package
|
||||
# Now safe since this module is named memory_manager (not memory)
|
||||
from memory import save_session_insights as save_file_based_memory
|
||||
from memory.graphiti_helpers import get_graphiti_memory
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
@@ -113,24 +114,20 @@ async def get_graphiti_context(
|
||||
debug("memory", "Graphiti not enabled, skipping context retrieval")
|
||||
return None
|
||||
|
||||
memory = None
|
||||
try:
|
||||
from graphiti_memory import GraphitiMemory
|
||||
|
||||
# Create memory manager
|
||||
memory = GraphitiMemory(spec_dir, project_dir)
|
||||
|
||||
if not memory.is_enabled:
|
||||
# Use centralized helper for GraphitiMemory instantiation
|
||||
memory = get_graphiti_memory(spec_dir, project_dir)
|
||||
if memory is None:
|
||||
if is_debug_enabled():
|
||||
debug_warning("memory", "GraphitiMemory.is_enabled=False")
|
||||
debug_warning("memory", "GraphitiMemory not available")
|
||||
return None
|
||||
|
||||
# Build search query from subtask description
|
||||
subtask_desc = subtask.get("description", "")
|
||||
subtask_id = subtask.get("id", "")
|
||||
query = f"{subtask_desc} {subtask_id}".strip()
|
||||
|
||||
if not query:
|
||||
await memory.close()
|
||||
if is_debug_enabled():
|
||||
debug_warning("memory", "Empty query, skipping context retrieval")
|
||||
return None
|
||||
@@ -155,8 +152,6 @@ async def get_graphiti_context(
|
||||
# Also get recent session history
|
||||
session_history = await memory.get_session_history(limit=3)
|
||||
|
||||
await memory.close()
|
||||
|
||||
if is_debug_enabled():
|
||||
debug(
|
||||
"memory",
|
||||
@@ -229,14 +224,20 @@ async def get_graphiti_context(
|
||||
|
||||
return "\n".join(sections)
|
||||
|
||||
except ImportError:
|
||||
logger.debug("Graphiti packages not installed")
|
||||
if is_debug_enabled():
|
||||
debug_warning("memory", "Graphiti packages not installed")
|
||||
return None
|
||||
except Exception as e:
|
||||
logger.warning(f"Failed to get Graphiti context: {e}")
|
||||
if is_debug_enabled():
|
||||
debug_error("memory", "Graphiti context retrieval failed", error=str(e))
|
||||
return None
|
||||
finally:
|
||||
# Always close the memory connection (swallow exceptions to avoid overriding)
|
||||
if memory is not None:
|
||||
try:
|
||||
await memory.close()
|
||||
except Exception as e:
|
||||
logger.debug(
|
||||
"Failed to close Graphiti memory connection", exc_info=True
|
||||
)
|
||||
|
||||
|
||||
async def save_session_memory(
|
||||
@@ -321,20 +322,24 @@ async def save_session_memory(
|
||||
if is_debug_enabled():
|
||||
debug("memory", "Attempting PRIMARY storage: Graphiti")
|
||||
|
||||
memory = None
|
||||
try:
|
||||
from graphiti_memory import GraphitiMemory
|
||||
# Use centralized helper for GraphitiMemory instantiation
|
||||
memory = get_graphiti_memory(spec_dir, project_dir)
|
||||
if memory is None:
|
||||
if is_debug_enabled():
|
||||
debug_warning("memory", "GraphitiMemory not available")
|
||||
# Continue to file-based fallback
|
||||
else:
|
||||
if is_debug_enabled():
|
||||
debug_detailed(
|
||||
"memory",
|
||||
"GraphitiMemory instance created",
|
||||
is_enabled=memory.is_enabled,
|
||||
group_id=getattr(memory, "group_id", "unknown"),
|
||||
)
|
||||
|
||||
memory = GraphitiMemory(spec_dir, project_dir)
|
||||
|
||||
if is_debug_enabled():
|
||||
debug_detailed(
|
||||
"memory",
|
||||
"GraphitiMemory instance created",
|
||||
is_enabled=memory.is_enabled,
|
||||
group_id=getattr(memory, "group_id", "unknown"),
|
||||
)
|
||||
|
||||
if memory.is_enabled:
|
||||
if memory is not None and memory.is_enabled:
|
||||
if is_debug_enabled():
|
||||
debug("memory", "Saving to Graphiti...")
|
||||
|
||||
@@ -351,8 +356,6 @@ async def save_session_memory(
|
||||
# Fallback to basic session insights
|
||||
result = await memory.save_session_insights(session_num, insights)
|
||||
|
||||
await memory.close()
|
||||
|
||||
if result:
|
||||
logger.info(
|
||||
f"Session {session_num} insights saved to Graphiti (primary)"
|
||||
@@ -373,23 +376,32 @@ async def save_session_memory(
|
||||
debug_warning(
|
||||
"memory", "Graphiti save returned False, using FALLBACK"
|
||||
)
|
||||
else:
|
||||
logger.warning(
|
||||
"Graphiti memory not enabled, falling back to file-based"
|
||||
)
|
||||
elif memory is None:
|
||||
if is_debug_enabled():
|
||||
debug_warning(
|
||||
"memory", "GraphitiMemory.is_enabled=False, using FALLBACK"
|
||||
"memory", "GraphitiMemory not available, using FALLBACK"
|
||||
)
|
||||
else:
|
||||
# memory is not None but memory.is_enabled is False
|
||||
logger.warning(
|
||||
"GraphitiMemory.is_enabled=False, falling back to file-based"
|
||||
)
|
||||
if is_debug_enabled():
|
||||
debug_warning("memory", "GraphitiMemory disabled, using FALLBACK")
|
||||
|
||||
except ImportError as e:
|
||||
logger.debug("Graphiti packages not installed, falling back to file-based")
|
||||
if is_debug_enabled():
|
||||
debug_warning("memory", "Graphiti packages not installed", error=str(e))
|
||||
except Exception as e:
|
||||
logger.warning(f"Graphiti save failed: {e}, falling back to file-based")
|
||||
if is_debug_enabled():
|
||||
debug_error("memory", "Graphiti save failed", error=str(e))
|
||||
finally:
|
||||
# Always close the memory connection (swallow exceptions to avoid overriding)
|
||||
if memory is not None:
|
||||
try:
|
||||
await memory.close()
|
||||
except Exception as e:
|
||||
logger.debug(
|
||||
"Failed to close Graphiti memory connection", exc_info=e
|
||||
)
|
||||
else:
|
||||
if is_debug_enabled():
|
||||
debug("memory", "Graphiti not enabled, skipping to FALLBACK")
|
||||
|
||||
@@ -48,15 +48,14 @@ async def _save_to_graphiti_async(
|
||||
True if save succeeded, False otherwise
|
||||
"""
|
||||
try:
|
||||
# Check if Graphiti is enabled
|
||||
from graphiti_config import is_graphiti_enabled
|
||||
# Use centralized helper for GraphitiMemory instantiation
|
||||
# The helper handles enablement checks internally
|
||||
from memory.graphiti_helpers import get_graphiti_memory
|
||||
|
||||
if not is_graphiti_enabled():
|
||||
memory = get_graphiti_memory(spec_dir, project_dir)
|
||||
if memory is None:
|
||||
return False
|
||||
|
||||
from integrations.graphiti.queries_pkg.graphiti import GraphitiMemory
|
||||
|
||||
memory = GraphitiMemory(spec_dir, project_dir)
|
||||
try:
|
||||
if save_type == "discovery":
|
||||
# Save as codebase discovery
|
||||
@@ -77,11 +76,14 @@ async def _save_to_graphiti_async(
|
||||
result = False
|
||||
return result
|
||||
finally:
|
||||
await memory.close()
|
||||
# Always close the memory connection (swallow exceptions to avoid overriding)
|
||||
try:
|
||||
await memory.close()
|
||||
except Exception as e:
|
||||
logger.debug(
|
||||
"Failed to close Graphiti memory connection", exc_info=True
|
||||
)
|
||||
|
||||
except ImportError as e:
|
||||
logger.debug(f"Graphiti not available for memory tools: {e}")
|
||||
return False
|
||||
except Exception as e:
|
||||
logger.warning(f"Failed to save to Graphiti: {e}")
|
||||
return False
|
||||
|
||||
@@ -50,7 +50,7 @@ from .queries_pkg.schema import (
|
||||
def get_graphiti_memory(
|
||||
spec_dir: Path,
|
||||
project_dir: Path,
|
||||
group_id_mode: str = GroupIdMode.SPEC,
|
||||
group_id_mode: str = GroupIdMode.PROJECT,
|
||||
) -> GraphitiMemory:
|
||||
"""
|
||||
Get a GraphitiMemory instance for the given spec.
|
||||
@@ -60,10 +60,14 @@ def get_graphiti_memory(
|
||||
Args:
|
||||
spec_dir: Spec directory
|
||||
project_dir: Project root directory
|
||||
group_id_mode: "spec" for isolated memory, "project" for shared
|
||||
group_id_mode: "spec" for isolated memory, "project" for shared (default)
|
||||
|
||||
Returns:
|
||||
GraphitiMemory instance
|
||||
|
||||
Note:
|
||||
Default changed from SPEC to PROJECT to enable cross-spec learning across
|
||||
the entire project. Use GroupIdMode.SPEC explicitly for isolated per-spec memory.
|
||||
"""
|
||||
return GraphitiMemory(spec_dir, project_dir, group_id_mode)
|
||||
|
||||
|
||||
@@ -10,10 +10,13 @@ Handles checking if Graphiti is available and managing async operations.
|
||||
import asyncio
|
||||
import logging
|
||||
from pathlib import Path
|
||||
from typing import Any
|
||||
from typing import TYPE_CHECKING, Any
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
if TYPE_CHECKING:
|
||||
from graphiti_memory import GraphitiMemory
|
||||
|
||||
|
||||
def is_graphiti_memory_enabled() -> bool:
|
||||
"""
|
||||
@@ -34,7 +37,9 @@ def is_graphiti_memory_enabled() -> bool:
|
||||
return False
|
||||
|
||||
|
||||
def get_graphiti_memory(spec_dir: Path, project_dir: Path | None = None):
|
||||
def get_graphiti_memory(
|
||||
spec_dir: Path, project_dir: Path | None = None
|
||||
) -> "GraphitiMemory | None":
|
||||
"""
|
||||
Get a GraphitiMemory instance if available.
|
||||
|
||||
@@ -49,11 +54,12 @@ def get_graphiti_memory(spec_dir: Path, project_dir: Path | None = None):
|
||||
return None
|
||||
|
||||
try:
|
||||
from graphiti_memory import GraphitiMemory
|
||||
from graphiti_memory import GraphitiMemory, GroupIdMode
|
||||
|
||||
if project_dir is None:
|
||||
project_dir = spec_dir.parent.parent
|
||||
return GraphitiMemory(spec_dir, project_dir)
|
||||
# Use project-wide shared memory for cross-spec learning
|
||||
return GraphitiMemory(spec_dir, project_dir, group_id_mode=GroupIdMode.PROJECT)
|
||||
except ImportError:
|
||||
return None
|
||||
|
||||
@@ -100,7 +106,7 @@ async def save_to_graphiti_async(
|
||||
True if save succeeded, False otherwise
|
||||
"""
|
||||
graphiti = get_graphiti_memory(spec_dir, project_dir)
|
||||
if not graphiti:
|
||||
if graphiti is None:
|
||||
return False
|
||||
|
||||
try:
|
||||
@@ -120,13 +126,17 @@ async def save_to_graphiti_async(
|
||||
for gotcha in discoveries.get("gotchas_encountered", []):
|
||||
await graphiti.save_gotcha(gotcha)
|
||||
|
||||
await graphiti.close()
|
||||
return result
|
||||
|
||||
except Exception as e:
|
||||
logger.warning(f"Failed to save to Graphiti: {e}")
|
||||
try:
|
||||
await graphiti.close()
|
||||
except Exception:
|
||||
pass
|
||||
return False
|
||||
finally:
|
||||
# Always close the graphiti connection (swallow exceptions to avoid overriding)
|
||||
if graphiti is not None:
|
||||
try:
|
||||
await graphiti.close()
|
||||
except Exception as e:
|
||||
logger.debug(
|
||||
"Failed to close Graphiti memory connection", exc_info=True
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user