[pull] main from danny-avila:main

src/tools/__tests__/CloudflareSandboxExecution.test.ts

@@ -2,6 +2,7 @@ import type * as t from '@/types';
 import {
   createCloudflareWorkspaceFS,
   createCloudflareLocalExecutionConfig,
+  execWithClientTimeout,
   executeCloudflareBash,
   executeCloudflareCode,
 } from '../cloudflare/CloudflareSandboxExecutionEngine';
@@ -257,6 +258,136 @@ describe('Cloudflare sandbox execution backend', () => {
     expect(result.timedOut).toBe(true);
   });
 
+  it('rejects with a client-side timeout when sandbox exec stalls (no native cancellation)', async () => {
+    // The native Cloudflare Sandbox DO exec() is uncancellable (ExecOptions has no
+    // signal) and its own `timeout` is not enforced when the container/RPC stalls,
+    // while the in-sandbox `timeout(1)` wrapper only bounds a *running* command.
+    // Without a client-side race a stalled exec hangs until the host's run-level
+    // abort, burning the whole budget on one tool call (issue #251).
+    jest.useFakeTimers();
+    try {
+      let mainExecCalls = 0;
+      const sandbox = createRuntime({
+        exec: (command) => {
+          // Cleanup (`rm -rf`) resolves immediately; the real command stalls,
+          // simulating an unresponsive / cold container exec that never returns.
+          if (command.startsWith('rm -rf')) {
+            return Promise.resolve({ exitCode: 0, stdout: '', stderr: '' });
+          }
+          mainExecCalls += 1;
+          return new Promise<t.CloudflareSandboxExecResult>(() => undefined);
+        },
+      });
+
+      const promise = executeCloudflareCode(
+        { lang: 'py', code: 'print("slow")' },
+        { sandbox, workspaceRoot: '/workspace', timeoutMs: 1000 }
+      );
+      const assertion = expect(promise).rejects.toThrow(/client-side timeout/);
+
+      // Client backstop = outerTimeoutMs(1000) + 5000 = 11000ms; advance past it.
+      await jest.advanceTimersByTimeAsync(11500);
+      await assertion;
+      expect(mainExecCalls).toBe(1);
+    } finally {
+      jest.useRealTimers();
+    }
+  });
+
+  it('aborts signal-aware execs when the client timeout fires', async () => {
+    // For signal-aware transports (e.g. the HTTP bridge), a client timeout should
+    // actually cancel the underlying exec, not just abandon it.
+    jest.useFakeTimers();
+    try {
+      let mainSignal: AbortSignal | undefined;
+      const sandbox = createRuntime({
+        supportsExecSignal: true,
+        exec: (command, options) => {
+          if (command.startsWith('rm -rf')) {
+            return Promise.resolve({ exitCode: 0, stdout: '', stderr: '' });
+          }
+          mainSignal = options?.signal;
+          return new Promise<t.CloudflareSandboxExecResult>(() => undefined);
+        },
+      });
+
+      const promise = executeCloudflareCode(
+        { lang: 'py', code: 'print("slow")' },
+        { sandbox, workspaceRoot: '/workspace', timeoutMs: 1000 }
+      );
+      const assertion = expect(promise).rejects.toThrow(/client-side timeout/);
+      await jest.advanceTimersByTimeAsync(11500);
+      await assertion;
+
+      expect(mainSignal).toBeDefined();
+      expect(mainSignal?.aborted).toBe(true);
+    } finally {
+      jest.useRealTimers();
+    }
+  });
+
+  it('composes a caller abort signal with the timeout instead of clobbering it', async () => {
+    let received: AbortSignal | undefined;
+    const sandbox = createRuntime({
+      supportsExecSignal: true,
+      exec: (_command, options) => {
+        received = options?.signal;
+        return new Promise<t.CloudflareSandboxExecResult>(
+          (_resolve, reject) => {
+            options?.signal?.addEventListener(
+              'abort',
+              () => reject(new Error('aborted')),
+              { once: true }
+            );
+          }
+        );
+      },
+    });
+
+    const caller = new AbortController();
+    const settled = execWithClientTimeout(
+      sandbox,
+      'echo hi',
+      { signal: caller.signal },
+      60000,
+      'test'
+    ).catch((e) => e as Error);
+
+    await Promise.resolve();
+    expect(received).toBeDefined();
+    // The exec gets a composed signal, not the caller's directly.
+    expect(received).not.toBe(caller.signal);
+    expect(received?.aborted).toBe(false);
+
+    // A caller cancellation must reach the exec (not wait for the client timeout).
+    caller.abort();
+    await settled;
+    expect(received?.aborted).toBe(true);
+  });
+
+  it('strips a caller signal for native runtimes that cannot consume it', async () => {
+    let received: t.CloudflareSandboxExecOptions | undefined;
+    const sandbox = createRuntime({
+      // no supportsExecSignal -> native DO, which cannot clone/consume a signal
+      exec: async (_command, options) => {
+        received = options;
+        return { exitCode: 0, stdout: 'ok', stderr: '' };
+      },
+    });
+    const caller = new AbortController();
+
+    await execWithClientTimeout(
+      sandbox,
+      'echo hi',
+      { cwd: '/workspace', signal: caller.signal },
+      60000,
+      'test'
+    );
+
+    expect(received).toBeDefined();
+    expect(received).not.toHaveProperty('signal');
+  });
+
   it('passes call-specific timeouts to the Cloudflare spawn wrapper', async () => {
     let execCommand = '';
     let execTimeout: number | undefined;

src/tools/cloudflare/CloudflareProgrammaticToolCalling.ts

@@ -19,6 +19,8 @@ import {
 } from '@/tools/BashProgrammaticToolCalling';
 import { Constants } from '@/common';
 import {
+  clientExecTimeoutMs,
+  execWithClientTimeout,
   executeCloudflareCode,
   getCloudflareWorkspaceRoot,
   resolveCloudflareSandbox,
@@ -166,13 +168,16 @@ async function executeGeneratedCloudflareBash(
   const workspaceRoot = getCloudflareWorkspaceRoot(config);
   const shell = config.shell ?? 'bash';
   const timeoutMs = config.timeoutMs ?? DEFAULT_TIMEOUT;
-  const result = await sandbox.exec(
+  const result = await execWithClientTimeout(
+    sandbox,
     withInSandboxTimeout(`${shell} -lc ${quoteShell(command)}`, timeoutMs),
     {
       cwd: workspaceRoot,
       env: config.env,
       timeout: outerTimeoutMs(timeoutMs),
-    }
+    },
+    clientExecTimeoutMs(timeoutMs),
+    'cloudflare bash programmatic exec'
   );
   const maxOutputChars = config.maxOutputChars ?? DEFAULT_MAX_OUTPUT_CHARS;
   return {

src/tools/cloudflare/CloudflareSandboxExecutionEngine.ts

@@ -131,6 +131,135 @@ function isInSandboxTimeoutExit(exitCode: number | null): boolean {
   return exitCode === 124 || exitCode === 137;
 }
 
+/**
+ * Client-side backstop timeout for a `sandbox.exec()` await: a few seconds beyond
+ * the exec's own `timeout` option, so a stalled exec that never honors `timeout`
+ * still can't outlast this.
+ */
+export function clientExecTimeoutMs(timeoutMs: number): number {
+  return outerTimeoutMs(timeoutMs) + 5000;
+}
+
+/**
+ * Bound a `sandbox.exec()` await with a CLIENT-SIDE timeout.
+ *
+ * The native Cloudflare Sandbox Durable Object `exec()` is effectively
+ * uncancellable from the host: `ExecOptions` has no `signal` (so
+ * `supportsExecSignal` is false for the native transport), and its `timeout`
+ * option is not reliably enforced when the container/RPC itself stalls — while
+ * the in-sandbox `timeout(1)` wrapper only bounds a command that is actually
+ * running. So a stalled exec (an unresponsive/cold container) otherwise hangs
+ * until the host's run-level abort, burning the whole run budget on one tool
+ * call. This race guarantees the host await settles within `timeoutMs`
+ * regardless of the transport.
+ *
+ * On timeout the underlying `exec` promise may keep running in the DO (a
+ * native-DO exec cannot be truly cancelled), so its late settlement is swallowed
+ * to avoid an unhandled rejection.
+ */
+export async function withClientTimeout<T>(
+  exec: Promise<T>,
+  timeoutMs: number,
+  label: string,
+  options: {
+    /**
+     * Detach the backstop timer from the event loop. Use ONLY when something else
+     * already settles the caller (e.g. the spawn path, where spawnLocalProcess's
+     * own timer resolves the child). The awaited direct-exec paths must leave it
+     * REF'd so the timeout is guaranteed to fire even if nothing else is pending.
+     */
+    unref?: boolean;
+    /** Invoked when the client timeout fires — e.g. abort a signal-aware exec. */
+    onTimeout?: () => void;
+  } = {}
+): Promise<T> {
+  if (!Number.isFinite(timeoutMs) || timeoutMs <= 0) {
+    return exec;
+  }
+  // Swallow a late rejection from the losing promise after the race settles.
+  exec.catch(() => undefined);
+  let timer: ReturnType<typeof setTimeout> | undefined;
+  try {
+    return await Promise.race([
+      exec,
+      new Promise<never>((_resolve, reject) => {
+        timer = setTimeout(() => {
+          // Reject FIRST so this client-timeout message reliably wins the race;
+          // only then abort a signal-aware exec, whose resulting AbortError must
+          // not surface to the caller instead of the timeout.
+          reject(
+            new Error(
+              `${label} exceeded ${timeoutMs}ms client-side timeout (sandbox exec did not return)`
+            )
+          );
+          options.onTimeout?.();
+        }, timeoutMs);
+        if (options.unref === true) {
+          (timer as { unref?: () => void } | undefined)?.unref?.();
+        }
+      }),
+    ]);
+  } finally {
+    if (timer !== undefined) {
+      clearTimeout(timer);
+    }
+  }
+}
+
+/**
+ * Run `sandbox.exec()` bounded by a client-side timeout, and — for signal-aware
+ * transports (e.g. the HTTP bridge, `supportsExecSignal === true`) — abort the
+ * underlying exec when the timeout fires instead of merely abandoning it. The
+ * native DO transport ignores `signal`, so it only gets the timeout. Leaves the
+ * backstop timer ref'd (this is an awaited direct-exec path).
+ */
+export async function execWithClientTimeout(
+  sandbox: t.CloudflareSandboxRuntime,
+  command: string,
+  options: t.CloudflareSandboxExecOptions,
+  timeoutMs: number,
+  label: string,
+  runOptions: { unref?: boolean } = {}
+): Promise<t.CloudflareSandboxExecResult> {
+  const controller = new AbortController();
+  const execOptions: t.CloudflareSandboxExecOptions = { ...options };
+  const callerSignal = options.signal;
+  let onCallerAbort: (() => void) | undefined;
+  if (sandbox.supportsExecSignal === true) {
+    // Compose the caller's signal (e.g. run/user cancellation) with our timeout
+    // controller so EITHER source cancels the exec — don't clobber the caller's.
+    if (callerSignal != null) {
+      if (callerSignal.aborted) {
+        controller.abort();
+      } else {
+        onCallerAbort = (): void => controller.abort();
+        callerSignal.addEventListener('abort', onCallerAbort, { once: true });
+      }
+    }
+    execOptions.signal = controller.signal;
+  } else if ('signal' in execOptions) {
+    // Native DO RPC cannot consume an AbortSignal (and would fail to clone it).
+    // Strip any caller-provided one so the spread above can't reintroduce it.
+    delete execOptions.signal;
+  }
+  try {
+    return await withClientTimeout(
+      sandbox.exec(command, execOptions),
+      timeoutMs,
+      label,
+      {
+        unref: runOptions.unref,
+        onTimeout: () => controller.abort(),
+      }
+    );
+  } finally {
+    // Don't leave a listener attached to a long-lived/shared caller signal.
+    if (onCallerAbort != null && callerSignal != null) {
+      callerSignal.removeEventListener('abort', onCallerAbort);
+    }
+  }
+}
+
 function truncateOutput(value: string, maxChars: number): string {
   if (maxChars <= 0 || value.length <= maxChars) {
     return value;
@@ -466,7 +595,14 @@ function createCloudflareSpawn(
         execOptions.signal = abortController.signal;
       }
       try {
-        const result = await ctx.sandbox.exec(timedCommand, execOptions);
+        const result = await withClientTimeout(
+          ctx.sandbox.exec(timedCommand, execOptions),
+          clientExecTimeoutMs(timeoutMs),
+          'cloudflare sandbox exec',
+          // spawnLocalProcess's own timer already resolves the child, so this
+          // backstop may safely detach; abort the (signal-aware) exec on timeout.
+          { unref: true, onTimeout: () => abortController.abort() }
+        );
         if (isClosed()) {
           return;
         }
@@ -551,13 +687,16 @@ export async function executeCloudflareBash(
     args.length > 0
       ? `${ctx.shell} -lc ${quote(command)} -- ${args.map(quote).join(' ')}`
       : `${ctx.shell} -lc ${quote(command)}`;
-  const result = await ctx.sandbox.exec(
+  const result = await execWithClientTimeout(
+    ctx.sandbox,
     withInSandboxTimeout(shellCommand, ctx.timeoutMs),
     {
       cwd: ctx.workspaceRoot,
       env: ctx.env,
       timeout: outerTimeoutMs(ctx.timeoutMs),
-    }
+    },
+    clientExecTimeoutMs(ctx.timeoutMs),
+    'cloudflare sandbox bash exec'
   );
   return {
     stdout: truncateOutput(result.stdout, ctx.maxOutputChars),
@@ -703,29 +842,46 @@ export async function executeCloudflareCode(
       }
     );
   }
+  let execSucceeded = false;
   try {
-    const result = await ctx.sandbox.exec(
+    const result = await execWithClientTimeout(
+      ctx.sandbox,
       withInSandboxTimeout(runtime.command, ctx.timeoutMs),
       {
         cwd: ctx.workspaceRoot,
         env: ctx.env,
         timeout: outerTimeoutMs(ctx.timeoutMs),
-      }
+      },
+      clientExecTimeoutMs(ctx.timeoutMs),
+      'cloudflare sandbox code-exec'
     );
+    execSucceeded = true;
     return {
       stdout: truncateOutput(result.stdout, ctx.maxOutputChars),
       stderr: truncateOutput(result.stderr, ctx.maxOutputChars),
       exitCode: result.exitCode,
       timedOut: isInSandboxTimeoutExit(result.exitCode),
     };
   } finally {
-    await ctx.sandbox
-      .exec(`rm -rf ${quote(tempDir)}`, {
+    // After a normal run, AWAIT cleanup so the temp dir is gone before returning.
+    // After a stalled/failed run, detach it (unref'd) so we don't pile a second
+    // client timeout onto the caller's latency; cleanup still runs best-effort.
+    const detach = !execSucceeded;
+    const cleanup = execWithClientTimeout(
+      ctx.sandbox,
+      `rm -rf ${quote(tempDir)}`,
+      {
         cwd: ctx.workspaceRoot,
         env: ctx.env,
         timeout: 10000,
-      })
-      .catch(() => undefined);
+      },
+      clientExecTimeoutMs(10000),
+      'cloudflare sandbox cleanup',
+      { unref: detach }
+    ).catch(() => undefined);
+    if (!detach) {
+      await cleanup;
+    }
   }
 }