Browse Source

fix(grep): harden fallback cache and byte streams

Avoid permanently caching transient GNU grep validation failures, and replace repeated Uint8Array concatenation in byte-stream parsers with a growable buffer to remove O(n²) chunk copying.
dhaern 3 months ago
parent
commit
1af798ea52

+ 41 - 0
src/tools/grep/aggregate.test.ts

@@ -3,7 +3,9 @@ import { describe, expect, test } from 'bun:test';
 import { GrepAggregator } from './aggregate';
 import { GrepAggregator } from './aggregate';
 import {
 import {
   consumeNullCountPairs,
   consumeNullCountPairs,
+  consumeNullCountPairsBytes,
   consumeNullItems,
   consumeNullItems,
+  consumeNullItemsBytes,
   consumeRgJsonStream,
   consumeRgJsonStream,
 } from './json-stream';
 } from './json-stream';
 import { createTempTracker, createTextStream } from './test-helpers';
 import { createTempTracker, createTextStream } from './test-helpers';
@@ -225,6 +227,45 @@ describe('tools/grep/aggregate', () => {
     await run();
     await run();
   });
   });
 
 
+  test.each([
+    {
+      name: 'consumeNullItemsBytes stitches chunked filenames and drops trailing partial bytes',
+      run: async () => {
+        const decoder = new TextDecoder();
+        const items: string[] = [];
+
+        await consumeNullItemsBytes(
+          createTextStream(['al', 'pha\0be', 'ta\0gam']),
+          (item) => {
+            items.push(decoder.decode(item));
+            return true;
+          },
+        );
+
+        expect(items).toEqual(['alpha', 'beta']);
+      },
+    },
+    {
+      name: 'consumeNullCountPairsBytes stitches chunked pairs without copying each chunk into a new buffer',
+      run: async () => {
+        const decoder = new TextDecoder();
+        const pairs: Array<[string, string]> = [];
+
+        await consumeNullCountPairsBytes(
+          createTextStream(['al', 'pha\x001', '2\nbe', 'ta\x003oops']),
+          (filePath, countText) => {
+            pairs.push([decoder.decode(filePath), countText]);
+            return true;
+          },
+        );
+
+        expect(pairs).toEqual([['alpha', '12']]);
+      },
+    },
+  ])('$name', async ({ run }) => {
+    await run();
+  });
+
   test('consumeRgJsonStream respects false returned by trailing callback', async () => {
   test('consumeRgJsonStream respects false returned by trailing callback', async () => {
     const events: string[] = [];
     const events: string[] = [];
     await consumeRgJsonStream(
     await consumeRgJsonStream(

+ 62 - 0
src/tools/grep/fallback.test.ts

@@ -196,4 +196,66 @@ describe('tools/grep/fallback', () => {
     expect(matched?.matchCount).toBe(2);
     expect(matched?.matchCount).toBe(2);
     expect(result.totalMatches).toBe(2);
     expect(result.totalMatches).toBe(2);
   });
   });
+
+  test('executeGrepFallback retries GNU grep validation after transient failures', async () => {
+    const repoDir = temps.createRepo();
+    const wrapperDir = temps.createDir('oh-my-opencode-grep-wrapper');
+    const markerPath = path.join(wrapperDir, 'validated');
+    const wrapperPath = path.join(wrapperDir, 'grep-wrapper.sh');
+
+    writeFileSync(
+      wrapperPath,
+      [
+        '#!/usr/bin/env bash',
+        'set -eu',
+        `marker=${JSON.stringify(markerPath)}`,
+        'if [ "$#" -gt 0 ] && [ "$1" = "--version" ]; then',
+        '  if [ ! -f "$marker" ]; then',
+        '    : > "$marker"',
+        "    printf 'resource temporarily unavailable\\n' >&2",
+        '    exit 1',
+        '  fi',
+        "  printf 'grep (GNU grep) 3.11\\n'",
+        '  exit 0',
+        'fi',
+        'exec grep "$@"',
+        '',
+      ].join('\n'),
+      { mode: 0o755 },
+    );
+
+    const input = normalizeGrepInput(
+      {
+        pattern: 'createTool',
+        path: path.join(repoDir, 'src'),
+        output_mode: 'files_with_matches',
+        fixed_strings: true,
+      },
+      createRepoContext(repoDir) as any,
+    );
+    const cli = {
+      path: wrapperPath,
+      backend: 'grep' as const,
+      source: 'system-gnu-grep' as const,
+    };
+
+    const first = await executeGrepFallback(
+      input,
+      new AbortController().signal,
+      cli,
+    );
+    expect(first.error).toContain('resource temporarily unavailable');
+
+    const second = await executeGrepFallback(
+      input,
+      new AbortController().signal,
+      cli,
+    );
+    expect(second.error).toBeUndefined();
+    expect(
+      second.files.some(
+        (file) => file.absolutePath === path.join(repoDir, 'src', 'example.ts'),
+      ),
+    ).toBe(true);
+  });
 });
 });

+ 118 - 29
src/tools/grep/fallback.ts

@@ -27,6 +27,8 @@ import {
   createFriendlySpawnError,
   createFriendlySpawnError,
   type GrepProcess,
   type GrepProcess,
   getAbortKind,
   getAbortKind,
+  isTransientFailure,
+  isTransientStderr,
   killProcess,
   killProcess,
   spawnRipgrep,
   spawnRipgrep,
   toErrorMessage,
   toErrorMessage,
@@ -55,7 +57,46 @@ function isSimpleBasenameGlob(glob: string): boolean {
   return !glob.includes('/') && !glob.includes('\\') && !glob.includes('**');
   return !glob.includes('/') && !glob.includes('\\') && !glob.includes('**');
 }
 }
 
 
-const GNU_GREP_CACHE = new Map<string, Promise<string | undefined>>();
+interface GnuGrepCheckResult {
+  error?: string;
+  cacheable: boolean;
+}
+
+const GNU_GREP_CACHE = new Map<string, Promise<GnuGrepCheckResult>>();
+
+function isAbortLikeFailure(error: unknown): boolean {
+  if (!(error instanceof Error)) {
+    return false;
+  }
+
+  const message = error.message.toLowerCase();
+  return (
+    error.name === 'AbortError' ||
+    message.includes('aborted') ||
+    message.includes('cancelled') ||
+    message.includes('canceled')
+  );
+}
+
+function shouldCacheGnuGrepFailure(
+  error: unknown,
+  stderr?: string,
+  firstLine?: string,
+): boolean {
+  if (isTransientFailure(error) || isAbortLikeFailure(error)) {
+    return false;
+  }
+
+  if (stderr && isTransientStderr(stderr)) {
+    return false;
+  }
+
+  if (firstLine !== undefined && firstLine.length === 0) {
+    return false;
+  }
+
+  return true;
+}
 
 
 function toWebReadableStream(
 function toWebReadableStream(
   stream: NodeJS.ReadableStream | ReadableStream<Uint8Array> | undefined,
   stream: NodeJS.ReadableStream | ReadableStream<Uint8Array> | undefined,
@@ -591,42 +632,90 @@ function finalizeFiles(
   };
   };
 }
 }
 
 
-async function ensureGnuGrep(binaryPath: string): Promise<string | undefined> {
-  const cached = GNU_GREP_CACHE.get(binaryPath);
-  if (cached) {
-    return cached;
+async function checkGnuGrep(
+  binaryPath: string,
+): Promise<GnuGrepCheckResult> {
+  let proc: GrepProcess;
+  try {
+    proc = spawnRipgrep([binaryPath, '--version'], process.cwd());
+  } catch (error) {
+    return {
+      error: toErrorMessage(error),
+      cacheable: shouldCacheGnuGrepFailure(error),
+    };
   }
   }
 
 
-  const check = (async () => {
-    let proc: GrepProcess;
-    try {
-      proc = spawnRipgrep([binaryPath, '--version'], process.cwd());
-    } catch (error) {
-      return toErrorMessage(error);
-    }
+  const stdoutPromise = readTextStream(proc.proc.stdout ?? undefined);
+  const stderrPromise = readTextStream(proc.proc.stderr ?? undefined);
+  const [stdoutResult, exitResult] = await Promise.allSettled([
+    stdoutPromise,
+    waitForExitAndStderr(proc, stderrPromise),
+  ]);
 
 
-    const stdoutPromise = readTextStream(proc.proc.stdout ?? undefined);
-    const stderrPromise = readTextStream(proc.proc.stderr ?? undefined);
-    const { exitCode, stderr } = await waitForExitAndStderr(
-      proc,
-      stderrPromise,
-    );
-    const stdout = (await stdoutPromise).trim();
+  if (stdoutResult.status === 'rejected') {
+    return {
+      error: toErrorMessage(stdoutResult.reason),
+      cacheable: shouldCacheGnuGrepFailure(stdoutResult.reason),
+    };
+  }
 
 
-    if (exitCode !== 0) {
-      return stderr || `grep --version exited with code ${String(exitCode)}`;
-    }
+  if (exitResult.status === 'rejected') {
+    return {
+      error: toErrorMessage(exitResult.reason),
+      cacheable: shouldCacheGnuGrepFailure(exitResult.reason),
+    };
+  }
 
 
-    const firstLine = stdout.split(/\r?\n/, 1)[0] ?? '';
-    if (!firstLine.includes('GNU grep')) {
-      return 'System grep fallback requires GNU grep; the detected grep is not GNU grep.';
-    }
+  const stdout = stdoutResult.value.trim();
+  const { exitCode, stderr } = exitResult.value;
 
 
-    return undefined;
-  })();
+  if (exitCode !== 0) {
+    const error = stderr || `grep --version exited with code ${String(exitCode)}`;
+    return {
+      error,
+      cacheable: shouldCacheGnuGrepFailure(undefined, stderr),
+    };
+  }
+
+  const firstLine = stdout.split(/\r?\n/, 1)[0] ?? '';
+  if (!firstLine.includes('GNU grep')) {
+    return {
+      error:
+        firstLine.length > 0
+          ? 'System grep fallback requires GNU grep; the detected grep is not GNU grep.'
+          : 'System grep fallback could not validate GNU grep version output.',
+      cacheable: shouldCacheGnuGrepFailure(undefined, undefined, firstLine),
+    };
+  }
+
+  return {
+    cacheable: true,
+  };
+}
+
+async function ensureGnuGrep(binaryPath: string): Promise<string | undefined> {
+  const cached = GNU_GREP_CACHE.get(binaryPath);
+  if (cached) {
+    return (await cached).error;
+  }
+
+  const check = checkGnuGrep(binaryPath);
 
 
   GNU_GREP_CACHE.set(binaryPath, check);
   GNU_GREP_CACHE.set(binaryPath, check);
-  return check;
+  void check.then(
+    (result) => {
+      if (!result.cacheable && GNU_GREP_CACHE.get(binaryPath) === check) {
+        GNU_GREP_CACHE.delete(binaryPath);
+      }
+    },
+    () => {
+      if (GNU_GREP_CACHE.get(binaryPath) === check) {
+        GNU_GREP_CACHE.delete(binaryPath);
+      }
+    },
+  );
+
+  return (await check).error;
 }
 }
 
 
 export async function executeGrepFallback(
 export async function executeGrepFallback(

+ 100 - 28
src/tools/grep/json-stream.ts

@@ -52,6 +52,94 @@ function toWebReadableStream(
   ) as unknown as ReadableStream<Uint8Array>;
   ) as unknown as ReadableStream<Uint8Array>;
 }
 }
 
 
+class GrowableByteBuffer {
+  private buffer = new Uint8Array(0);
+  private start = 0;
+  private end = 0;
+  private searchStart = 0;
+
+  append(chunk?: Uint8Array): void {
+    if (!chunk || chunk.length === 0) {
+      return;
+    }
+
+    this.ensureCapacity(chunk.length);
+    this.buffer.set(chunk, this.end);
+    this.end += chunk.length;
+  }
+
+  takeUntil(delimiter: number): Uint8Array | undefined {
+    const scanStart = Math.max(this.start, this.searchStart);
+    const relativeIndex = this.buffer
+      .subarray(scanStart, this.end)
+      .indexOf(delimiter);
+    if (relativeIndex < 0) {
+      this.searchStart = this.end;
+      return undefined;
+    }
+
+    const absoluteIndex = scanStart + relativeIndex;
+    const item = this.buffer.slice(this.start, absoluteIndex);
+    this.start = absoluteIndex + 1;
+    this.searchStart = this.start;
+    this.compactIfNeeded();
+    return item;
+  }
+
+  private ensureCapacity(additional: number): void {
+    const currentLength = this.end - this.start;
+    const requiredLength = currentLength + additional;
+
+    if (this.buffer.length === 0) {
+      this.buffer = new Uint8Array(Math.max(64, requiredLength));
+      return;
+    }
+
+    if (requiredLength <= this.buffer.length) {
+      if (this.end + additional <= this.buffer.length) {
+        return;
+      }
+
+      const previousStart = this.start;
+      this.buffer.copyWithin(0, this.start, this.end);
+      this.start = 0;
+      this.end = currentLength;
+      this.searchStart = Math.max(0, this.searchStart - previousStart);
+      return;
+    }
+
+    const next = new Uint8Array(
+      Math.max(this.buffer.length * 2, requiredLength),
+    );
+    next.set(this.buffer.subarray(this.start, this.end), 0);
+    this.searchStart = Math.max(0, this.searchStart - this.start);
+    this.buffer = next;
+    this.start = 0;
+    this.end = currentLength;
+  }
+
+  private compactIfNeeded(): void {
+    const currentLength = this.end - this.start;
+
+    if (currentLength === 0) {
+      this.start = 0;
+      this.end = 0;
+      this.searchStart = 0;
+      return;
+    }
+
+    if (this.start < this.buffer.length / 2) {
+      return;
+    }
+
+    const previousStart = this.start;
+    this.buffer.copyWithin(0, this.start, this.end);
+    this.start = 0;
+    this.end = currentLength;
+    this.searchStart = Math.max(0, this.searchStart - previousStart);
+  }
+}
+
 async function consumeDelimitedText(
 async function consumeDelimitedText(
   stream: BinaryReadableStream,
   stream: BinaryReadableStream,
   delimiter: string,
   delimiter: string,
@@ -128,28 +216,20 @@ export async function consumeNullItemsBytes(
   }
   }
 
 
   const reader = readable.getReader();
   const reader = readable.getReader();
-  let buffer = new Uint8Array();
+  const buffer = new GrowableByteBuffer();
 
 
   while (true) {
   while (true) {
     const { done, value } = await reader.read();
     const { done, value } = await reader.read();
-    if (value) {
-      const next = new Uint8Array(buffer.length + value.length);
-      next.set(buffer);
-      next.set(value, buffer.length);
-      buffer = next;
-    }
-
-    let index = buffer.indexOf(0);
-    while (index >= 0) {
-      const item = buffer.slice(0, index);
-      buffer = buffer.slice(index + 1);
+    buffer.append(value);
 
 
+    let item = buffer.takeUntil(0);
+    while (item !== undefined) {
       if (onItem(item) === false) {
       if (onItem(item) === false) {
         await reader.cancel();
         await reader.cancel();
         return;
         return;
       }
       }
 
 
-      index = buffer.indexOf(0);
+      item = buffer.takeUntil(0);
     }
     }
 
 
     if (done) {
     if (done) {
@@ -223,37 +303,29 @@ export async function consumeNullCountPairsBytes(
 
 
   const reader = readable.getReader();
   const reader = readable.getReader();
   const decoder = new TextDecoder();
   const decoder = new TextDecoder();
-  let buffer = new Uint8Array();
+  const buffer = new GrowableByteBuffer();
   let currentPath: Uint8Array | undefined;
   let currentPath: Uint8Array | undefined;
 
 
   while (true) {
   while (true) {
     const { done, value } = await reader.read();
     const { done, value } = await reader.read();
-    if (value) {
-      const next = new Uint8Array(buffer.length + value.length);
-      next.set(buffer);
-      next.set(value, buffer.length);
-      buffer = next;
-    }
+    buffer.append(value);
 
 
     while (true) {
     while (true) {
       if (currentPath === undefined) {
       if (currentPath === undefined) {
-        const nullIndex = buffer.indexOf(0);
-        if (nullIndex < 0) {
+        const pathBytes = buffer.takeUntil(0);
+        if (pathBytes === undefined) {
           break;
           break;
         }
         }
 
 
-        currentPath = buffer.slice(0, nullIndex);
-        buffer = buffer.slice(nullIndex + 1);
+        currentPath = pathBytes;
         continue;
         continue;
       }
       }
 
 
-      const newlineIndex = buffer.indexOf(0x0a);
-      if (newlineIndex < 0) {
+      const countBytes = buffer.takeUntil(0x0a);
+      if (countBytes === undefined) {
         break;
         break;
       }
       }
 
 
-      const countBytes = buffer.slice(0, newlineIndex);
-      buffer = buffer.slice(newlineIndex + 1);
       const pathBytes = currentPath;
       const pathBytes = currentPath;
       currentPath = undefined;
       currentPath = undefined;
       const countText = decoder.decode(countBytes).replace(/\r$/, '');
       const countText = decoder.decode(countBytes).replace(/\r$/, '');