fix: address reviewer concerns about timeout and recursion convergence (PR CortexReach#215 follow-up)

This commit addresses the two blocking issues raised in PR CortexReach#215:

1. Timeout now uses AbortController for TRUE request cancellation
   - Timer is properly cleaned up in .finally()
   - AbortSignal is passed through to embedWithRetry

2. Recursion now guarantees monotonic convergence
   - Introduced STRICT_REDUCTION_FACTOR = 0.5
   - Each recursion level must reduce input by 50%
   - Works regardless of model context size

Modified by AI assistant (not human code) based on PR CortexReach#215.
Thanks to the original author and maintainers.

Co-authored-by: Hi-Jiajun <Hi-Jiajun@users.noreply.github.com>

Author: 2 people

src/embedder.ts

@@ -258,6 +258,12 @@ const MAX_EMBED_DEPTH = 3;
 /** Global timeout for a single embedding operation (ms). */
 const EMBED_TIMEOUT_MS = 10_000;
 
+/**
+ * Strictly decreasing character limit for forced truncation.
+ * Each recursion level MUST reduce input by this factor to guarantee progress.
+ */
+const STRICT_REDUCTION_FACTOR = 0.5; // Each retry must be at most 50% of previous
+
 /**
  * Safe character limits per model for forced truncation.
  * CJK characters typically consume ~3 tokens each, so the char limit is
@@ -382,16 +388,23 @@ export class Embedder {
   /**
    * Call embeddings.create with automatic key rotation on rate-limit errors.
    * Tries each key in the pool at most once before giving up.
+   * Accepts an optional AbortSignal to support true request cancellation.
    */
-  private async embedWithRetry(payload: any): Promise<any> {
+  private async embedWithRetry(payload: any, signal?: AbortSignal): Promise<any> {
     const maxAttempts = this.clients.length;
     let lastError: Error | undefined;
 
     for (let attempt = 0; attempt < maxAttempts; attempt++) {
       const client = this.nextClient();
       try {
-        return await client.embeddings.create(payload);
+        // Pass signal to OpenAI SDK if provided (SDK v6+ supports this)
+        return await client.embeddings.create(payload, signal ? { signal } : undefined);
       } catch (error) {
+        // If aborted, re-throw immediately
+        if (error instanceof Error && error.name === 'AbortError') {
+          throw error;
+        }
+        
         lastError = error instanceof Error ? error : new Error(String(error));
 
         if (this.isRateLimitError(error) && attempt < maxAttempts - 1) {
@@ -420,19 +433,33 @@ export class Embedder {
     return this.clients.length;
   }
 
-  /** FR-05: Wrap a promise with a global timeout to prevent indefinite hangs. */
-  private withTimeout<T>(promise: Promise<T>, label: string): Promise<T> {
-    return Promise.race([
-      promise,
-      new Promise<never>((_, reject) => {
-        setTimeout(
-          () => reject(new Error(
-            `[memory-lancedb-pro] ${label} timed out after ${EMBED_TIMEOUT_MS}ms`
-          )),
-          EMBED_TIMEOUT_MS,
-        );
-      }),
-    ]);
+  /** FR-05: Wrap a promise with a global timeout using AbortSignal for TRUE cancellation.
+   * @param promiseFactory - A function that receives an AbortSignal and returns a promise
+   */
+  private withTimeout<T>(promiseFactory: (signal: AbortSignal) => Promise<T>, label: string): Promise<T> {
+    const controller = new AbortController();
+    const timeoutId = setTimeout(() => controller.abort(), EMBED_TIMEOUT_MS);
+    
+    // Create the promise with the signal
+    const promise = promiseFactory(controller.signal);
+    
+    // Race between the original promise and timeout
+    // When timeout fires, controller.abort() will:
+    // 1. Trigger the abort event below to reject
+    // 2. If embedWithRetry received the signal, it will cancel the underlying HTTP request
+    const timeoutPromise = new Promise<never>((_, reject) => {
+      controller.signal.addEventListener('abort', () => {
+        clearTimeout(timeoutId);
+        reject(new Error(
+          `[memory-lancedb-pro] ${label} timed out after ${EMBED_TIMEOUT_MS}ms`
+        ));
+      });
+    });
+    
+    return Promise.race([promise, timeoutPromise])
+      .finally(() => {
+        clearTimeout(timeoutId);
+      }) as Promise<T>;
   }
 
   // --------------------------------------------------------------------------
@@ -459,11 +486,11 @@ export class Embedder {
   // --------------------------------------------------------------------------
 
   async embedQuery(text: string): Promise<number[]> {
-    return this.withTimeout(this.embedSingle(text, this._taskQuery), "embedQuery");
+    return this.withTimeout((signal) => this.embedSingle(text, this._taskQuery, signal), "embedQuery");
   }
 
   async embedPassage(text: string): Promise<number[]> {
-    return this.withTimeout(this.embedSingle(text, this._taskPassage), "embedPassage");
+    return this.withTimeout((signal) => this.embedSingle(text, this._taskPassage, signal), "embedPassage");
   }
 
   async embedBatchQuery(texts: string[]): Promise<number[][]> {
@@ -510,18 +537,23 @@ export class Embedder {
     return payload;
   }
 
-  private async embedSingle(text: string, task?: string, depth: number = 0): Promise<number[]> {
+  private async embedSingle(text: string, task?: string, depth: number = 0, signal?: AbortSignal): Promise<number[]> {
     if (!text || text.trim().length === 0) {
       throw new Error("Cannot embed empty text");
     }
 
     // FR-01: Recursion depth limit — force truncate when too deep
     if (depth >= MAX_EMBED_DEPTH) {
-      const safeLimit = getSafeCharLimit(this._model);
+      const safeLimit = Math.floor(text.length * STRICT_REDUCTION_FACTOR);
       console.warn(
         `[memory-lancedb-pro] Recursion depth ${depth} reached MAX_EMBED_DEPTH (${MAX_EMBED_DEPTH}), ` +
-        `force-truncating ${text.length} chars → ${safeLimit} chars`
+        `force-truncating ${text.length} chars → ${safeLimit} chars (strict ${STRICT_REDUCTION_FACTOR * 100}% reduction)`
       );
+      if (safeLimit < 100) {
+        throw new Error(
+          `[memory-lancedb-pro] Failed to embed: input too large for model context after ${MAX_EMBED_DEPTH} retries`
+        );
+      }
       text = text.slice(0, safeLimit);
     }
 
@@ -530,7 +562,7 @@ export class Embedder {
     if (cached) return cached;
 
     try {
-      const response = await this.embedWithRetry(this.buildPayload(text, task));
+      const response = await this.embedWithRetry(this.buildPayload(text, task), signal);
       const embedding = response.data[0]?.embedding as number[] | undefined;
       if (!embedding) {
         throw new Error("No embedding returned from provider");
@@ -555,27 +587,33 @@ export class Embedder {
 
           // FR-03: Single chunk output detection — if smartChunk produced only
           // one chunk that is nearly the same size as the original text, chunking
-          // did not actually reduce the problem.  Force-truncate instead of
-          // recursing (which would loop forever).
+          // did not actually reduce the problem. Force-truncate with STRICT
+          // reduction to guarantee progress.
           if (
             chunkResult.chunks.length === 1 &&
             chunkResult.chunks[0].length > text.length * 0.9
           ) {
-            const safeLimit = getSafeCharLimit(this._model);
+            // Use strict reduction factor to guarantee each retry makes progress
+            const safeLimit = Math.floor(text.length * STRICT_REDUCTION_FACTOR);
             console.warn(
               `[memory-lancedb-pro] smartChunk produced 1 chunk (${chunkResult.chunks[0].length} chars) ≈ original (${text.length} chars). ` +
-              `Force-truncating to ${safeLimit} chars to avoid infinite recursion.`
+              `Force-truncating to ${safeLimit} chars (strict ${STRICT_REDUCTION_FACTOR * 100}% reduction) to avoid infinite recursion.`
             );
+            if (safeLimit < 100) {
+              throw new Error(
+                `[memory-lancedb-pro] Failed to embed: chunking couldn't reduce input size enough for model context`
+              );
+            }
             const truncated = text.slice(0, safeLimit);
-            return this.embedSingle(truncated, task, depth + 1);
+            return this.embedSingle(truncated, task, depth + 1, signal);
           }
 
           // Embed all chunks in parallel
           console.log(`Split document into ${chunkResult.chunkCount} chunks for embedding`);
           const chunkEmbeddings = await Promise.all(
             chunkResult.chunks.map(async (chunk, idx) => {
               try {
-                const embedding = await this.embedSingle(chunk, task, depth + 1);
+                const embedding = await this.embedSingle(chunk, task, depth + 1, signal);
                 return { embedding };
               } catch (chunkError) {
                 console.warn(`Failed to embed chunk ${idx}:`, chunkError);