Merge pull request #63 from BillChirico/fix/mem0-review-fixes

Author: BillChirico

src/commands/memory.js

@@ -25,10 +25,10 @@ import {
 } from 'discord.js';
 import { info, warn } from '../logger.js';
 import {
+  checkAndRecoverMemory,
   deleteAllMemories,
   deleteMemory,
   getMemories,
-  isMemoryAvailable,
   searchMemories,
 } from '../modules/memory.js';
 import { isOptedOut, toggleOptOut } from '../modules/optout.js';
@@ -113,7 +113,7 @@ export async function execute(interaction) {
     return;
   }
 
-  if (!isMemoryAvailable()) {
+  if (!checkAndRecoverMemory()) {
     await interaction.reply({
       content:
         '🧠 Memory system is currently unavailable. The bot still works, just without long-term memory.',
@@ -261,26 +261,45 @@ async function handleForgetAll(interaction, userId, username) {
 async function handleForgetTopic(interaction, userId, username, topic) {
   await interaction.deferReply({ ephemeral: true });
 
-  // Search for memories matching the topic (results include IDs)
-  const { memories: matches } = await searchMemories(userId, topic, 10);
+  const BATCH_SIZE = 100;
+  const MAX_ITERATIONS = 10;
+  let totalDeleted = 0;
+  let totalFound = 0;
+  let iterations = 0;
 
-  if (matches.length === 0) {
-    await interaction.editReply({
-      content: `🔍 No memories found matching "${topic}".`,
-    });
-    return;
-  }
+  // Loop to delete all matching memories (not just the first batch)
+  while (iterations < MAX_ITERATIONS) {
+    iterations++;
+    const { memories: matches } = await searchMemories(userId, topic, BATCH_SIZE);
+
+    if (matches.length === 0) break;
+    totalFound += matches.length;
+
+    const matchesWithIds = matches.filter(
+      (m) => m.id !== undefined && m.id !== null && m.id !== '',
+    );
 
-  // Use memory IDs directly from search results and delete in parallel
-  const matchesWithIds = matches.filter((m) => m.id);
-  const results = await Promise.allSettled(matchesWithIds.map((m) => deleteMemory(m.id)));
-  const deletedCount = results.filter((r) => r.status === 'fulfilled' && r.value === true).length;
+    if (matchesWithIds.length === 0) break;
 
-  if (deletedCount > 0) {
+    const results = await Promise.allSettled(matchesWithIds.map((m) => deleteMemory(m.id)));
+    const batchDeleted = results.filter((r) => r.status === 'fulfilled' && r.value === true).length;
+    totalDeleted += batchDeleted;
+
+    // If we got fewer results than the batch size, we've reached the end
+    if (matches.length < BATCH_SIZE) break;
+    // If nothing was deleted this round, stop to avoid infinite loop
+    if (batchDeleted === 0) break;
+  }
+
+  if (totalDeleted > 0) {
     await interaction.editReply({
-      content: `🧹 Forgot ${deletedCount} memor${deletedCount === 1 ? 'y' : 'ies'} related to "${topic}".`,
+      content: `🧹 Forgot ${totalDeleted} memor${totalDeleted === 1 ? 'y' : 'ies'} related to "${topic}".`,
+    });
+    info('Topic memories cleared', { userId, username, topic, count: totalDeleted });
+  } else if (totalFound === 0) {
+    await interaction.editReply({
+      content: `🔍 No memories found matching "${topic}".`,
     });
-    info('Topic memories cleared', { userId, username, topic, count: deletedCount });
   } else {
     await interaction.editReply({
       content: `❌ Found memories about "${topic}" but couldn't delete them. Please try again.`,
@@ -309,7 +328,7 @@ async function handleAdmin(interaction, subcommand) {
     return;
   }
 
-  if (!isMemoryAvailable()) {
+  if (!checkAndRecoverMemory()) {
     await interaction.reply({
       content:
         '🧠 Memory system is currently unavailable. The bot still works, just without long-term memory.',

src/index.js

@@ -28,7 +28,7 @@ import {
 } from './modules/ai.js';
 import { loadConfig } from './modules/config.js';
 import { registerEventHandlers } from './modules/events.js';
-import { checkMem0Health } from './modules/memory.js';
+import { checkMem0Health, markUnavailable } from './modules/memory.js';
 import { startTempbanScheduler, stopTempbanScheduler } from './modules/moderation.js';
 import { loadOptOuts } from './modules/optout.js';
 import { HealthMonitor } from './utils/health.js';
@@ -293,8 +293,26 @@ async function startup() {
   // Load opt-out preferences from DB before enabling memory features
   await loadOptOuts();
 
-  // Check mem0 availability for user memory features
-  await checkMem0Health();
+  // Check mem0 availability for user memory features (with timeout to avoid blocking startup).
+  // AbortController prevents a late-resolving health check from calling markAvailable()
+  // after the timeout has already called markUnavailable().
+  const healthAbort = new AbortController();
+  try {
+    await Promise.race([
+      checkMem0Health({ signal: healthAbort.signal }),
+      new Promise((_, reject) =>
+        setTimeout(() => {
+          healthAbort.abort();
+          reject(new Error('mem0 health check timed out'));
+        }, 10_000),
+      ),
+    ]);
+  } catch (err) {
+    markUnavailable();
+    warn('mem0 health check timed out or failed — continuing without memory features', {
+      error: err.message,
+    });
+  }
 
   // Register event handlers with live config reference
   registerEventHandlers(client, config, healthMonitor);

src/modules/ai.js

@@ -400,15 +400,20 @@ You're witty, knowledgeable about programming and tech, and always eager to help
 Keep responses concise and Discord-friendly (under 2000 chars).
 You can use Discord markdown formatting.`;
 
-  // Pre-response: inject user memory context into system prompt
+  // Pre-response: inject user memory context into system prompt (with timeout)
   if (userId) {
     try {
-      const memoryContext = await buildMemoryContext(userId, username, userMessage);
+      const memoryContext = await Promise.race([
+        buildMemoryContext(userId, username, userMessage),
+        new Promise((_, reject) =>
+          setTimeout(() => reject(new Error('Memory context timeout')), 5000),
+        ),
+      ]);
       if (memoryContext) {
         systemPrompt += memoryContext;
       }
     } catch (err) {
-      // Memory lookup failed — continue without it
+      // Memory lookup failed or timed out — continue without it
       logWarn('Memory context lookup failed', { userId, error: err.message });
     }
   }

src/modules/memory.js

@@ -99,7 +99,7 @@ let client = null;
  * After RECOVERY_COOLDOWN_MS, the next request will be allowed through
  * to check if the service has recovered.
  */
-function markUnavailable() {
+export function markUnavailable() {
   mem0Available = false;
   mem0UnavailableSince = Date.now();
 }
@@ -154,15 +154,30 @@ export function getMemoryConfig() {
 }
 
 /**
- * Check if memory feature is enabled and mem0 is available.
- * Supports auto-recovery: if mem0 was marked unavailable due to a transient
- * error and the cooldown period has elapsed, it will be tentatively re-enabled
- * so the next request can check if the service has recovered.
+ * Pure availability check — no side effects.
+ * Returns true only if memory is both enabled in config and currently marked available.
+ * Does NOT trigger auto-recovery. Use {@link checkAndRecoverMemory} when you want
+ * the cooldown-based recovery logic.
  * @returns {boolean}
  */
 export function isMemoryAvailable() {
   const memConfig = getMemoryConfig();
   if (!memConfig.enabled) return false;
+  return mem0Available;
+}
+
+/**
+ * Check if memory feature is enabled and mem0 is available, with auto-recovery.
+ * If mem0 was marked unavailable due to a transient error and the cooldown period
+ * has elapsed, this will tentatively re-enable it so the next request can check
+ * if the service has recovered.
+ *
+ * Use this instead of {@link isMemoryAvailable} when you want the recovery side effect.
+ * @returns {boolean}
+ */
+export function checkAndRecoverMemory() {
+  const memConfig = getMemoryConfig();
+  if (!memConfig.enabled) return false;
 
   if (mem0Available) return true;
 
@@ -231,9 +246,12 @@ export function _setClient(newClient) {
  * Run a health check against the mem0 platform on startup.
  * Verifies the API key is configured and the SDK client can actually
  * communicate with the hosted platform by performing a lightweight search.
+ * @param {object} [options]
+ * @param {AbortSignal} [options.signal] - When aborted, prevents a late-resolving
+ *   check from calling {@link markAvailable} (guards against race with startup timeout).
  * @returns {Promise<boolean>} true if mem0 is ready
  */
-export async function checkMem0Health() {
+export async function checkMem0Health({ signal } = {}) {
   const memConfig = getMemoryConfig();
   if (!memConfig.enabled) {
     info('Memory module disabled via config');
@@ -262,6 +280,11 @@ export async function checkMem0Health() {
       limit: 1,
     });
 
+    // Guard against late resolution after a startup timeout has already
+    // called markUnavailable().  If the caller's AbortSignal has fired,
+    // the timeout won the race and we must not flip availability back on.
+    if (signal?.aborted) return false;
+
     markAvailable();
     info('mem0 health check passed (SDK connectivity verified)');
     return true;
@@ -286,7 +309,7 @@ export async function checkMem0Health() {
  * @returns {Promise<boolean>} true if stored successfully
  */
 export async function addMemory(userId, text, metadata = {}) {
-  if (!isMemoryAvailable()) return false;
+  if (!checkAndRecoverMemory()) return false;
 
   try {
     const c = getClient();
@@ -318,7 +341,7 @@ export async function addMemory(userId, text, metadata = {}) {
  * @returns {Promise<{memories: Array<{memory: string, score?: number}>, relations: Array}>}
  */
 export async function searchMemories(userId, query, limit) {
-  if (!isMemoryAvailable()) return { memories: [], relations: [] };
+  if (!checkAndRecoverMemory()) return { memories: [], relations: [] };
 
   const memConfig = getMemoryConfig();
   const maxResults = limit ?? memConfig.maxContextMemories;
@@ -339,7 +362,7 @@ export async function searchMemories(userId, query, limit) {
     const relations = result?.relations || [];
 
     const memories = rawMemories.map((m) => ({
-      id: m.id || '',
+      id: m.id ?? '',
       memory: m.memory || m.text || m.content || '',
       score: m.score ?? null,
     }));
@@ -358,7 +381,7 @@ export async function searchMemories(userId, query, limit) {
  * @returns {Promise<Array<{id: string, memory: string}>>} All user memories
  */
 export async function getMemories(userId) {
-  if (!isMemoryAvailable()) return [];
+  if (!checkAndRecoverMemory()) return [];
 
   try {
     const c = getClient();
@@ -373,7 +396,7 @@ export async function getMemories(userId) {
     const memories = Array.isArray(result) ? result : result?.results || [];
 
     return memories.map((m) => ({
-      id: m.id || '',
+      id: m.id ?? '',
       memory: m.memory || m.text || m.content || '',
     }));
   } catch (err) {
@@ -389,7 +412,7 @@ export async function getMemories(userId) {
  * @returns {Promise<boolean>} true if deleted successfully
  */
 export async function deleteAllMemories(userId) {
-  if (!isMemoryAvailable()) return false;
+  if (!checkAndRecoverMemory()) return false;
 
   try {
     const c = getClient();
@@ -411,7 +434,7 @@ export async function deleteAllMemories(userId) {
  * @returns {Promise<boolean>} true if deleted successfully
  */
 export async function deleteMemory(memoryId) {
-  if (!isMemoryAvailable()) return false;
+  if (!checkAndRecoverMemory()) return false;
 
   try {
     const c = getClient();
@@ -435,21 +458,29 @@ export async function deleteMemory(memoryId) {
 export function formatRelations(relations) {
   if (!relations || relations.length === 0) return '';
 
-  const lines = relations.map((r) => `- ${r.source} → ${r.relationship} → ${r.target}`);
+  const lines = relations
+    .filter((r) => r.source && r.relationship && r.target)
+    .map((r) => `- ${r.source} → ${r.relationship} → ${r.target}`);
+
+  if (lines.length === 0) return '';
 
   return `\nRelationships:\n${lines.join('\n')}`;
 }
 
+/** Maximum characters for memory context injected into system prompt */
+const MAX_MEMORY_CONTEXT_CHARS = 2000;
+
 /**
  * Build a context string from user memories to inject into the system prompt.
  * Includes both regular memories and graph relations for richer context.
+ * Enforces a character budget to prevent oversized system prompts.
  * @param {string} userId - Discord user ID
  * @param {string} username - Display name
  * @param {string} query - The user's current message (for relevance search)
  * @returns {Promise<string>} Context string or empty string
  */
 export async function buildMemoryContext(userId, username, query) {
-  if (!isMemoryAvailable()) return '';
+  if (!checkAndRecoverMemory()) return '';
   if (isOptedOut(userId)) return '';
 
   const { memories, relations } = await searchMemories(userId, query);
@@ -468,6 +499,11 @@ export async function buildMemoryContext(userId, username, query) {
     context += relationsContext;
   }
 
+  // Enforce character budget to prevent oversized system prompts
+  if (context.length > MAX_MEMORY_CONTEXT_CHARS) {
+    context = `${context.substring(0, MAX_MEMORY_CONTEXT_CHARS)}...`;
+  }
+
   return context;
 }
 
@@ -482,7 +518,7 @@ export async function buildMemoryContext(userId, username, query) {
  * @returns {Promise<boolean>} true if any memories were stored
  */
 export async function extractAndStoreMemories(userId, username, userMessage, assistantReply) {
-  if (!isMemoryAvailable()) return false;
+  if (!checkAndRecoverMemory()) return false;
   if (isOptedOut(userId)) return false;
 
   const memConfig = getMemoryConfig();
@@ -493,13 +529,14 @@ export async function extractAndStoreMemories(userId, username, userMessage, ass
     if (!c) return false;
 
     const messages = [
-      { role: 'user', content: `${username}: ${userMessage}` },
+      { role: 'user', content: userMessage },
       { role: 'assistant', content: assistantReply },
     ];
 
     await c.add(messages, {
       user_id: userId,
       app_id: APP_ID,
+      metadata: { username },
       enable_graph: true,
     });
 
@@ -510,8 +547,10 @@ export async function extractAndStoreMemories(userId, username, userMessage, ass
     });
     return true;
   } catch (err) {
+    // Only log — do NOT call markUnavailable() here.
+    // This runs fire-and-forget in the background; a failure for one user's
+    // extraction should not disable the memory system for all other users.
     logWarn('Memory extraction failed', { userId, error: err.message });
-    if (!isTransientError(err)) markUnavailable();
     return false;
   }
 }