Skip to content
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/wet-falcons-hang.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@cloudflare/sandbox": patch
---

add auto timeout renewal and some health checks
71 changes: 29 additions & 42 deletions .github/workflows/claude-code-review.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,46 +22,33 @@ jobs:
issues: read
id-token: write

steps:
- name: Checkout repository
uses: actions/checkout@v4
with:
fetch-depth: 1

- name: Run Claude Code Review
id: claude-review
uses: anthropics/claude-code-action@v1
with:
anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }}
prompt: |
Review PR #${{ github.event.pull_request.number }}.

Read CLAUDE.md for project conventions and architecture patterns. Focus on substantive issues:
- Code quality, potential bugs, architecture alignment
- Testing coverage

BE CONCISE. Only report actual issues worth addressing - skip generic praise and obvious observations.
If the PR looks good, just say so briefly.

**Posting your review:**
1. First, check if you've already posted a review comment on this PR (find the comment ID):
```bash
COMMENT_ID=$(gh api repos/${{ github.repository }}/issues/${{ github.event.pull_request.number }}/comments --jq '.[] | select(.user.login == "claude[bot]" or .user.login == "github-actions[bot]") | select(.body | startswith("## Claude Code Review")) | .id' | head -1)
```

2. If COMMENT_ID exists (non-empty), UPDATE that comment:
```bash
gh api repos/${{ github.repository }}/issues/comments/$COMMENT_ID -X PATCH -f body="YOUR_REVIEW_CONTENT_HERE"
```

3. If COMMENT_ID is empty, create a new comment:
```bash
gh pr comment ${{ github.event.pull_request.number }} --body "YOUR_REVIEW_CONTENT_HERE" --repo ${{ github.repository }}
```

Start your review with "## Claude Code Review" heading so it can be identified and updated on subsequent runs.

# See https://github.com/anthropics/claude-code-action/blob/main/docs/usage.md
# or https://docs.claude.com/en/docs/claude-code/cli-reference for available options
claude_args: '--allowed-tools "Bash(gh issue view:*),Bash(gh search:*),Bash(gh issue list:*),Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr list:*),Bash(gh api:*)"'
# steps:
# - name: Checkout repository
# uses: actions/checkout@v4
# with:
# fetch-depth: 1

# - name: Run Claude Code Review
# id: claude-review
# uses: anthropics/claude-code-action@v1
# with:
# anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }}
# prompt: |
# REPO: ${{ github.repository }}
# PR NUMBER: ${{ github.event.pull_request.number }}

# Please review this pull request and provide feedback on:
# - Code quality and best practices
# - Potential bugs or issues
# - Performance considerations
# - Security concerns
# - Test coverage

# Use the repository's CLAUDE.md for guidance on style and conventions. Be constructive and helpful in your feedback.

# Use `gh pr comment` with your Bash tool to leave your review as a comment on the PR.

# # See https://github.com/anthropics/claude-code-action/blob/main/docs/usage.md
# # or https://docs.claude.com/en/docs/claude-code/cli-reference for available options
# claude_args: '--allowed-tools "Bash(gh issue view:*),Bash(gh search:*),Bash(gh issue list:*),Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr list:*)"'

2 changes: 1 addition & 1 deletion .github/workflows/pkg-pr-new.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ on:
pull_request:
types: [opened, synchronize, reopened]
paths:
- '!**/*.md'
- '**'
- '!.changeset/**'


Expand Down
38 changes: 35 additions & 3 deletions packages/sandbox/src/sandbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ export function getSandbox(
stub.setSleepAfter(options.sleepAfter);
}

if (options?.keepAlive !== undefined) {
stub.setKeepAlive(options.keepAlive);
}

return stub;
}

Expand All @@ -64,6 +68,7 @@ export class Sandbox<Env = unknown> extends Container<Env> implements ISandbox {
private defaultSession: string | null = null;
envVars: Record<string, string> = {};
private logger: ReturnType<typeof createLogger>;
private keepAliveEnabled: boolean = false;

constructor(ctx: DurableObject['ctx'], env: Env) {
super(ctx, env);
Expand Down Expand Up @@ -131,6 +136,16 @@ export class Sandbox<Env = unknown> extends Container<Env> implements ISandbox {
this.sleepAfter = sleepAfter;
}

// RPC method to enable keepAlive mode
async setKeepAlive(keepAlive: boolean): Promise<void> {
this.keepAliveEnabled = keepAlive;
if (keepAlive) {
this.logger.info('KeepAlive mode enabled - container will stay alive until explicitly destroyed');
} else {
this.logger.info('KeepAlive mode disabled - container will timeout normally');
}
}

// RPC method to set environment variables
async setEnvVars(envVars: Record<string, string>): Promise<void> {
// Update local state for new sessions
Expand Down Expand Up @@ -220,6 +235,22 @@ export class Sandbox<Env = unknown> extends Container<Env> implements ISandbox {
this.logger.error('Sandbox error', error instanceof Error ? error : new Error(String(error)));
}

/**
* Override onActivityExpired to prevent automatic shutdown when keepAlive is enabled
* When keepAlive is disabled, calls parent implementation which stops the container
*/
override async onActivityExpired(): Promise<void> {
if (this.keepAliveEnabled) {
this.logger.debug('Activity expired but keepAlive is enabled - container will stay alive');
// Do nothing - don't call stop(), container stays alive
} else {
// Default behavior: stop the container
this.logger.debug('Activity expired - stopping container');
await super.onActivityExpired();
}
}


// Override fetch to route internal container requests to appropriate ports
override async fetch(request: Request): Promise<Response> {
// Extract or generate trace ID from request
Expand Down Expand Up @@ -327,7 +358,6 @@ export class Sandbox<Env = unknown> extends Container<Env> implements ISandbox {
const startTime = Date.now();
const timestamp = new Date().toISOString();

// Handle timeout
let timeoutId: NodeJS.Timeout | undefined;

try {
Expand Down Expand Up @@ -592,8 +622,7 @@ export class Sandbox<Env = unknown> extends Container<Env> implements ISandbox {
};
}


// Streaming methods - return ReadableStream for RPC compatibility
// Streaming methods - return ReadableStream for RPC compatibility
async execStream(command: string, options?: StreamOptions): Promise<ReadableStream<Uint8Array>> {
// Check for cancellation
if (options?.signal?.aborted) {
Expand All @@ -617,6 +646,9 @@ export class Sandbox<Env = unknown> extends Container<Env> implements ISandbox {
return this.client.commands.executeStream(command, sessionId);
}

/**
* Stream logs from a background process as a ReadableStream.
*/
async streamProcessLogs(processId: string, options?: { signal?: AbortSignal }): Promise<ReadableStream<Uint8Array>> {
// Check for cancellation
if (options?.signal?.aborted) {
Expand Down
39 changes: 39 additions & 0 deletions packages/sandbox/tests/get-sandbox.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ describe('getSandbox', () => {
setSleepAfter: vi.fn((value: string | number) => {
mockStub.sleepAfter = value;
}),
setKeepAlive: vi.fn(),
};

// Mock getContainer to return our stub
Expand Down Expand Up @@ -107,4 +108,42 @@ describe('getSandbox', () => {
expect(sandbox.sleepAfter).toBe(timeString);
}
});

it('should apply keepAlive option when provided as true', () => {
const mockNamespace = {} as any;
const sandbox = getSandbox(mockNamespace, 'test-sandbox', {
keepAlive: true,
});

expect(sandbox.setKeepAlive).toHaveBeenCalledWith(true);
});

it('should apply keepAlive option when provided as false', () => {
const mockNamespace = {} as any;
const sandbox = getSandbox(mockNamespace, 'test-sandbox', {
keepAlive: false,
});

expect(sandbox.setKeepAlive).toHaveBeenCalledWith(false);
});

it('should not call setKeepAlive when keepAlive option not provided', () => {
const mockNamespace = {} as any;
getSandbox(mockNamespace, 'test-sandbox');

expect(mockStub.setKeepAlive).not.toHaveBeenCalled();
});

it('should apply keepAlive alongside other options', () => {
const mockNamespace = {} as any;
const sandbox = getSandbox(mockNamespace, 'test-sandbox', {
sleepAfter: '5m',
baseUrl: 'https://example.com',
keepAlive: true,
});

expect(sandbox.sleepAfter).toBe('5m');
expect(sandbox.setBaseUrl).toHaveBeenCalledWith('https://example.com');
expect(sandbox.setKeepAlive).toHaveBeenCalledWith(true);
});
});
53 changes: 52 additions & 1 deletion packages/sandbox/tests/sandbox.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ describe('Sandbox - Automatic Session Management', () => {
delete: vi.fn().mockResolvedValue(undefined),
list: vi.fn().mockResolvedValue(new Map()),
} as any,
blockConcurrencyWhile: vi.fn((fn: () => Promise<void>) => fn()),
blockConcurrencyWhile: vi.fn().mockImplementation(<T>(callback: () => Promise<T>): Promise<T> => callback()),
id: {
toString: () => 'test-sandbox-id',
equals: vi.fn(),
Expand Down Expand Up @@ -476,6 +476,57 @@ describe('Sandbox - Automatic Session Management', () => {
});
});

describe('Health checks', () => {
beforeEach(() => {
// Mock Container base class getState method for health checks
(sandbox as any).getState = vi.fn().mockResolvedValue({
status: 'running',
timestamp: new Date().toISOString(),
});
});


it('should handle health check failures gracefully', async () => {
const encoder = new TextEncoder();
const mockStream = new ReadableStream({
start(controller) {
controller.enqueue(encoder.encode('test\n'));
controller.close();
}
});

vi.spyOn(sandbox.client.commands, 'executeStream').mockResolvedValue(mockStream);

// Make getState throw an error (container is dead)
(sandbox as any).getState = vi.fn().mockRejectedValue(new Error('Container not found'));

const stream = await sandbox.execStream('echo test');
const reader = stream.getReader();

// Read should still work since we're testing the health check error handling
// The health check runs in background and shouldn't block reads initially
const result = await reader.read();
expect(result.done).toBe(false);
});

it('should work with all streaming APIs', async () => {
const encoder = new TextEncoder();
const mockStream = new ReadableStream({
start(controller) {
controller.enqueue(encoder.encode('test\n'));
controller.close();
}
});

// Test execStream
vi.spyOn(sandbox.client.commands, 'executeStream').mockResolvedValue(mockStream);
const execStream = await sandbox.execStream('echo test');
expect(execStream).toBeInstanceOf(ReadableStream);

// Test streamProcessLogs
vi.spyOn(sandbox.client.processes, 'streamProcessLogs').mockResolvedValue(mockStream);
const logsStream = await sandbox.streamProcessLogs('proc-1');
expect(logsStream).toBeInstanceOf(ReadableStream);
describe('fetch() override - WebSocket detection', () => {
let superFetchSpy: any;

Expand Down
2 changes: 1 addition & 1 deletion packages/shared/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,9 @@ export type {
ExecOptions,
ExecResult,
ExecutionSession,
FileExistsResult,
// File streaming types
FileChunk,
FileExistsResult,
FileInfo,
FileMetadata,
FileStreamEvent,
Expand Down
17 changes: 15 additions & 2 deletions packages/shared/src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -262,13 +262,26 @@ export interface SandboxOptions {
* - A string like "30s", "3m", "5m", "1h" (seconds, minutes, or hours)
* - A number representing seconds (e.g., 180 for 3 minutes)
* Default: "10m" (10 minutes)
*
* Note: Ignored when keepAlive is true
*/
sleepAfter?: string | number;

/**
* Base URL for the sandbox API
*/
baseUrl?: string;

/**
* Keep the container alive indefinitely by preventing automatic shutdown
* When true, the container will never auto-timeout and must be explicitly destroyed
* - Any scenario where activity can't be automatically detected
*
* Important: You MUST call sandbox.destroy() when done to avoid resource leaks
*
* Default: false
*/
keepAlive?: boolean;
}

/**
Expand Down Expand Up @@ -590,7 +603,7 @@ export interface ExecutionSession {
// Command execution
exec(command: string, options?: ExecOptions): Promise<ExecResult>;
execStream(command: string, options?: StreamOptions): Promise<ReadableStream<Uint8Array>>;

// Background process management
startProcess(command: string, options?: ProcessOptions): Promise<Process>;
listProcesses(): Promise<Process[]>;
Expand Down Expand Up @@ -621,7 +634,7 @@ export interface ExecutionSession {
// Code interpreter methods
createCodeContext(options?: CreateContextOptions): Promise<CodeContext>;
runCode(code: string, options?: RunCodeOptions): Promise<ExecutionResult>;
runCodeStream(code: string, options?: RunCodeOptions): Promise<ReadableStream>;
runCodeStream(code: string, options?: RunCodeOptions): Promise<ReadableStream<Uint8Array>>;
listCodeContexts(): Promise<CodeContext[]>;
deleteCodeContext(contextId: string): Promise<void>;
}
Expand Down
Loading
Loading