Skip to content

Commit 485cf61

Browse files
ghostwriternrghostwriternr
authored
Various bug fixes (#108)
* Fix interprter streaming & env setting * Fix interpreter context pollution * Fix error propagation for code interrpreter * Add language validation * Handle stream after command completion * Fix setEnvVars to update existing sessions * Add retries for container startup delays * Increase timeout for e2e tests to 90s * Fix cleanup script * Improve logs * Fix cleanup script * Always run e2e sequentially
1 parent da947cd commit 485cf61

28 files changed

+441
-364
lines changed

‎.github/workflows/cleanup-stale.yml‎

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -79,17 +79,9 @@ jobs:
7979
8080
# Cleanup if needed
8181
if [ "$SHOULD_CLEANUP" = true ]; then
82-
echo " Deleting worker: $WORKER_NAME"
83-
npx wrangler delete --name "$WORKER_NAME" 2>/dev/null || echo " Failed to delete worker"
84-
85-
# Also delete associated container
86-
CONTAINER_ID=$(npx wrangler containers list --json 2>/dev/null | jq -r ".[] | select(.name==\"$WORKER_NAME\") | .id")
87-
if [ -n "$CONTAINER_ID" ]; then
88-
echo " Deleting container: $CONTAINER_ID"
89-
npx wrangler containers delete "$CONTAINER_ID" 2>/dev/null || echo " Failed to delete container"
90-
fi
91-
92-
echo " ✅ Cleanup complete"
82+
echo " Calling cleanup script for: $WORKER_NAME"
83+
# Call cleanup script (indent output for readability)
84+
../../../scripts/cleanup-test-deployment.sh "$WORKER_NAME" 2>&1 | sed 's/^/ /'
9385
fi
9486
done
9587

‎.github/workflows/cleanup.yml‎

Lines changed: 3 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -28,27 +28,11 @@ jobs:
2828
run: |
2929
echo "worker_name=sandbox-e2e-test-worker-pr-${{ github.event.pull_request.number }}" >> $GITHUB_OUTPUT
3030
31-
- name: Delete test worker
31+
- name: Cleanup test deployment
3232
continue-on-error: true
3333
run: |
34-
npx wrangler delete --name ${{ steps.worker-name.outputs.worker_name }} || echo "Worker already deleted or doesn't exist"
35-
working-directory: tests/e2e/test-worker
36-
env:
37-
CLOUDFLARE_API_TOKEN: ${{ secrets.CLOUDFLARE_API_TOKEN }}
38-
CLOUDFLARE_ACCOUNT_ID: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }}
39-
40-
- name: Delete test container
41-
continue-on-error: true
42-
run: |
43-
# Get container ID for this worker and delete it
44-
CONTAINER_ID=$(npx wrangler containers list --json | jq -r '.[] | select(.name=="${{ steps.worker-name.outputs.worker_name }}") | .id')
45-
if [ -n "$CONTAINER_ID" ]; then
46-
echo "Deleting container: $CONTAINER_ID"
47-
npx wrangler containers delete "$CONTAINER_ID" || echo "Failed to delete container"
48-
else
49-
echo "No container found for worker ${{ steps.worker-name.outputs.worker_name }}"
50-
fi
51-
working-directory: tests/e2e/test-worker
34+
cd tests/e2e/test-worker
35+
../../../scripts/cleanup-test-deployment.sh ${{ steps.worker-name.outputs.worker_name }}
5236
env:
5337
CLOUDFLARE_API_TOKEN: ${{ secrets.CLOUDFLARE_API_TOKEN }}
5438
CLOUDFLARE_ACCOUNT_ID: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }}

‎.github/workflows/pullrequest.yml‎

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -134,18 +134,8 @@ jobs:
134134
if: always() && github.event_name == 'pull_request'
135135
continue-on-error: true
136136
run: |
137-
# Delete worker (this also removes the worker's binding to the container)
138-
npx wrangler delete --name ${{ steps.env-name.outputs.worker_name }} || echo "Worker already deleted or doesn't exist"
139-
140-
# Get container ID for this worker and delete it
141-
CONTAINER_ID=$(npx wrangler containers list --json | jq -r '.[] | select(.name=="${{ steps.env-name.outputs.worker_name }}") | .id')
142-
if [ -n "$CONTAINER_ID" ]; then
143-
echo "Deleting container: $CONTAINER_ID"
144-
npx wrangler containers delete "$CONTAINER_ID" || echo "Failed to delete container"
145-
else
146-
echo "No container found for worker ${{ steps.env-name.outputs.worker_name }}"
147-
fi
148-
working-directory: tests/e2e/test-worker
137+
cd tests/e2e/test-worker
138+
../../../scripts/cleanup-test-deployment.sh ${{ steps.env-name.outputs.worker_name }}
149139
env:
150140
CLOUDFLARE_API_TOKEN: ${{ secrets.CLOUDFLARE_API_TOKEN }}
151141
CLOUDFLARE_ACCOUNT_ID: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }}

‎.github/workflows/release.yml‎

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -90,7 +90,9 @@ jobs:
9090
- name: Set worker name
9191
id: worker-name
9292
run: |
93-
echo "worker_name=sandbox-e2e-test-worker-release-${{ github.run_number }}" >> $GITHUB_OUTPUT
93+
# Use git SHA for unique, meaningful naming (not sequential run number)
94+
SHORT_SHA=$(echo "${{ github.sha }}" | cut -c1-7)
95+
echo "worker_name=sandbox-e2e-test-worker-release-${SHORT_SHA}" >> $GITHUB_OUTPUT
9496
9597
# Generate unique wrangler config for this release
9698
- name: Generate wrangler config
@@ -124,18 +126,8 @@ jobs:
124126
if: always()
125127
continue-on-error: true
126128
run: |
127-
# Delete worker (this also removes the worker's binding to the container)
128-
npx wrangler delete --name ${{ steps.worker-name.outputs.worker_name }} || echo "Worker already deleted or doesn't exist"
129-
130-
# Get container ID for this worker and delete it
131-
CONTAINER_ID=$(npx wrangler containers list --json | jq -r '.[] | select(.name=="${{ steps.worker-name.outputs.worker_name }}") | .id')
132-
if [ -n "$CONTAINER_ID" ]; then
133-
echo "Deleting container: $CONTAINER_ID"
134-
npx wrangler containers delete "$CONTAINER_ID" || echo "Failed to delete container"
135-
else
136-
echo "No container found for worker ${{ steps.worker-name.outputs.worker_name }}"
137-
fi
138-
working-directory: tests/e2e/test-worker
129+
cd tests/e2e/test-worker
130+
../../../scripts/cleanup-test-deployment.sh ${{ steps.worker-name.outputs.worker_name }}
139131
env:
140132
CLOUDFLARE_API_TOKEN: ${{ secrets.CLOUDFLARE_API_TOKEN }}
141133
CLOUDFLARE_ACCOUNT_ID: ${{ secrets.CLOUDFLARE_ACCOUNT_ID }}

‎examples/basic/src/endpoints/createTestBinary.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ print("Chart saved to /workspace/demo-chart.png")
4343
// Check for errors
4444
if (execution.error) {
4545
console.error("Error creating chart:", execution.error);
46-
return errorResponse(`Failed to create chart: ${execution.error.value}`);
46+
return errorResponse(`Failed to create chart: ${execution.error.message}`);
4747
}
4848

4949
// Return success with file path

‎packages/sandbox-container/src/config.ts‎

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11,18 +11,6 @@ const INTERPRETER_SPAWN_TIMEOUT_MS = parseInt(
1111
10,
1212
);
1313

14-
/**
15-
* Timeout for internal pre-warm scripts that warm up interpreter pools.
16-
* These are system-level operations that should complete quickly.
17-
*
18-
* Default: 30 seconds
19-
* Environment variable: INTERPRETER_PREWARM_TIMEOUT_MS
20-
*/
21-
const INTERPRETER_PREWARM_TIMEOUT_MS = parseInt(
22-
process.env.INTERPRETER_PREWARM_TIMEOUT_MS || '30000',
23-
10,
24-
);
25-
2614
/**
2715
* Timeout for interpreter code execution (Python/JS/TS).
2816
* Users might legitimately run long computations (ML training, data processing, etc.)
@@ -78,7 +66,6 @@ const DEFAULT_CWD = '/workspace';
7866

7967
export const CONFIG = {
8068
INTERPRETER_SPAWN_TIMEOUT_MS,
81-
INTERPRETER_PREWARM_TIMEOUT_MS,
8269
INTERPRETER_EXECUTION_TIMEOUT_MS,
8370
COMMAND_TIMEOUT_MS,
8471
MAX_OUTPUT_SIZE_BYTES,

‎packages/sandbox-container/src/handlers/process-handler.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -352,6 +352,19 @@ export class ProcessHandler extends BaseHandler<Request, Response> {
352352
process.outputListeners.add(outputListener);
353353
process.statusListeners.add(statusListener);
354354

355+
// If process already completed, send exit event immediately
356+
if (['completed', 'failed', 'killed', 'error'].includes(process.status)) {
357+
const finalData = `data: ${JSON.stringify({
358+
type: 'exit',
359+
processId: process.id,
360+
exitCode: process.exitCode,
361+
data: `Process ${process.status} with exit code ${process.exitCode}`,
362+
timestamp: new Date().toISOString(),
363+
})}\n\n`;
364+
controller.enqueue(new TextEncoder().encode(finalData));
365+
controller.close();
366+
}
367+
355368
// Cleanup when stream is cancelled
356369
return () => {
357370
process.outputListeners.delete(outputListener);

‎packages/sandbox-container/src/interpreter-service.ts‎

Lines changed: 28 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -120,21 +120,21 @@ export class InterpreterService {
120120
if (result.stdout) {
121121
controller.enqueue(
122122
encoder.encode(
123-
`${JSON.stringify({
123+
self.formatSSE({
124124
type: "stdout",
125125
text: result.stdout,
126-
})}\n`
126+
})
127127
)
128128
);
129129
}
130130

131131
if (result.stderr) {
132132
controller.enqueue(
133133
encoder.encode(
134-
`${JSON.stringify({
134+
self.formatSSE({
135135
type: "stderr",
136136
text: result.stderr,
137-
})}\n`
137+
})
138138
)
139139
);
140140
}
@@ -144,11 +144,11 @@ export class InterpreterService {
144144
const outputData = self.formatOutputData(output);
145145
controller.enqueue(
146146
encoder.encode(
147-
`${JSON.stringify({
147+
self.formatSSE({
148148
type: "result",
149149
...outputData,
150150
metadata: output.metadata || {},
151-
})}\n`
151+
})
152152
)
153153
);
154154
}
@@ -157,51 +157,51 @@ export class InterpreterService {
157157
if (result.success) {
158158
controller.enqueue(
159159
encoder.encode(
160-
`${JSON.stringify({
160+
self.formatSSE({
161161
type: "execution_complete",
162162
execution_count: 1,
163-
})}\n`
163+
})
164164
)
165165
);
166166
} else if (result.error) {
167167
controller.enqueue(
168168
encoder.encode(
169-
`${JSON.stringify({
169+
self.formatSSE({
170170
type: "error",
171171
ename: result.error.type || "ExecutionError",
172172
evalue: result.error.message || "Code execution failed",
173173
traceback: result.error.traceback ? result.error.traceback.split('\n') : [],
174-
})}\n`
174+
})
175175
)
176176
);
177177
} else {
178178
controller.enqueue(
179179
encoder.encode(
180-
`${JSON.stringify({
180+
self.formatSSE({
181181
type: "error",
182182
ename: "ExecutionError",
183183
evalue: result.stderr || "Code execution failed",
184184
traceback: [],
185-
})}\n`
185+
})
186186
)
187187
);
188188
}
189189

190190
controller.close();
191191
} catch (error) {
192192
console.error(`[InterpreterService] Code execution failed:`, error);
193-
193+
194194
controller.enqueue(
195195
encoder.encode(
196-
`${JSON.stringify({
196+
self.formatSSE({
197197
type: "error",
198198
ename: "InternalError",
199199
evalue: error instanceof Error ? error.message : String(error),
200200
traceback: [],
201-
})}\n`
201+
})
202202
)
203203
);
204-
204+
205205
controller.close();
206206
}
207207
},
@@ -240,7 +240,7 @@ export class InterpreterService {
240240

241241
private formatOutputData(output: RichOutput): Record<string, unknown> {
242242
const result: Record<string, unknown> = {};
243-
243+
244244
switch (output.type) {
245245
case "image":
246246
result.png = output.data;
@@ -272,7 +272,17 @@ export class InterpreterService {
272272
default:
273273
result.text = output.data || '';
274274
}
275-
275+
276276
return result;
277277
}
278+
279+
/**
280+
* Format event as SSE (Server-Sent Events)
281+
* SSE format requires "data: " prefix and double newline separator
282+
* @param event - Event object to send
283+
* @returns SSE-formatted string
284+
*/
285+
private formatSSE(event: Record<string, unknown>): string {
286+
return `data: ${JSON.stringify(event)}\n\n`;
287+
}
278288
}

‎packages/sandbox-container/src/runtime/process-pool.ts‎

Lines changed: 0 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@ export interface PoolConfig {
3636
maxProcesses: number;
3737
idleTimeout: number; // milliseconds
3838
minSize: number;
39-
preWarmScript?: string;
4039
}
4140

4241
export interface ExecutorPoolConfig extends PoolConfig {
@@ -49,38 +48,18 @@ const DEFAULT_EXECUTOR_CONFIGS: Record<InterpreterLanguage, ExecutorPoolConfig>
4948
minSize: 3,
5049
maxProcesses: 15,
5150
idleTimeout: 5 * 60 * 1000, // 5 minutes
52-
preWarmScript: `
53-
import json
54-
print(json.dumps({"status": "pre-warmed"}))
55-
`
5651
},
5752
javascript: {
5853
executor: "javascript",
5954
minSize: 3,
6055
maxProcesses: 10,
6156
idleTimeout: 5 * 60 * 1000,
62-
preWarmScript: `
63-
const fs = require('fs');
64-
const path = require('path');
65-
const util = require('util');
66-
const crypto = require('crypto');
67-
for(let i = 0; i < 1000; i++) {
68-
JSON.stringify({x: i, data: Math.random()});
69-
}
70-
console.log(JSON.stringify({"status": "pre-warmed"}));
71-
`
7257
},
7358
typescript: {
7459
executor: "typescript",
7560
minSize: 3,
7661
maxProcesses: 10,
7762
idleTimeout: 5 * 60 * 1000,
78-
preWarmScript: `
79-
const { transformSync } = require('esbuild');
80-
const warmupCode = 'interface Test { x: number; } const test: Test = { x: 42 }; test.x';
81-
transformSync(warmupCode, { loader: 'ts', target: 'es2020', format: 'cjs' });
82-
console.log(JSON.stringify({"status": "pre-warmed"}));
83-
`
8463
}
8564
};
8665

@@ -392,10 +371,6 @@ export class ProcessPoolManager {
392371
const sessionId = `pre-warm-${executor}-${i}-${Date.now()}`;
393372
const process = await this.createProcess(executor, sessionId);
394373

395-
if (config.preWarmScript) {
396-
await this.executePreWarmScript(process, config.preWarmScript, executor);
397-
}
398-
399374
process.isAvailable = true;
400375
process.sessionId = undefined;
401376
pool.push(process);
@@ -409,30 +384,6 @@ export class ProcessPoolManager {
409384
console.log(`[ProcessPool] Pre-warmed ${actualCount}/${config.minSize} ${executor} processes in ${warmupTime}ms`);
410385
}
411386

412-
private async executePreWarmScript(
413-
process: InterpreterProcess,
414-
script: string,
415-
executor: InterpreterLanguage
416-
): Promise<void> {
417-
try {
418-
const executionId = `pre-warm-${Date.now()}`;
419-
const result = await this.executeCode(
420-
process,
421-
script,
422-
executionId,
423-
CONFIG.INTERPRETER_PREWARM_TIMEOUT_MS
424-
);
425-
426-
if (result.success) {
427-
console.log(`[ProcessPool] ${executor} pre-warm script executed successfully`);
428-
} else {
429-
console.warn(`[ProcessPool] ${executor} pre-warm script failed:`, result.stderr);
430-
}
431-
} catch (error) {
432-
console.warn(`[ProcessPool] ${executor} pre-warm script error:`, error);
433-
}
434-
}
435-
436387
private cleanupIdleProcesses(): void {
437388
const now = new Date();
438389

0 commit comments

Comments
 (0)