Skip to content

Commit 9cda4a6

Browse files
Resolve all critical and high-priority Gemini review issues
Critical Issues Fixed: - Unified MCP and REST API into single process (main.py) - Moved KernelPool to core/kernel_pool.py, eliminating circular dependencies - Removed monkey-patching by adding proper KernelPool.execute_on_kernel method - Implemented proper async execution using FastAPI BackgroundTasks High Priority Issues Fixed: - Eliminated code duplication in execute_on_kernel functions - Replaced all bare except clauses with specific exception types - Fixed setup.py to use read_requirements() function Architecture Improvements: - Single unified app on port 8222 with REST API mounted on /api - Clean module structure with core/ components - Proper error handling throughout codebase - Functional async task execution
1 parent 02c72c2 commit 9cda4a6

File tree

10 files changed

+1252
-506
lines changed

10 files changed

+1252
-506
lines changed

__init__.py

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -55,13 +55,13 @@ def __init__(self, auto_start: bool = True, base_url: str = None, isolated: bool
5555
# Create isolated fresh container
5656
try:
5757
self.container_id, ports = ContainerManager.create_isolated_container()
58-
self.base_url = f"http://localhost:{ports['rest']}"
59-
logger.info(f"Created isolated container {self.container_id} on port {ports['rest']}")
58+
self.base_url = f"http://localhost:{ports['mcp']}/api"
59+
logger.info(f"Created isolated container {self.container_id} on port {ports['mcp']}")
6060
except Exception as e:
6161
raise CodeRunnerError(f"Failed to create isolated container: {e}")
6262
else:
63-
# Use shared container (backward compatibility)
64-
self.base_url = base_url or "http://localhost:8223"
63+
# Use shared container (backward compatibility) - now unified on port 8222
64+
self.base_url = base_url or "http://localhost:8222/api"
6565
if auto_start:
6666
try:
6767
ContainerManager.ensure_running()
@@ -106,7 +106,7 @@ def execute(self, code: str, language: str = "python", timeout: int = 30) -> Dic
106106
try:
107107
error_data = response.json()
108108
error_detail = error_data.get("detail", error_detail)
109-
except:
109+
except (ValueError, requests.exceptions.JSONDecodeError):
110110
error_detail = response.text
111111

112112
raise ExecutionError(f"Execution failed: {error_detail}")
@@ -159,7 +159,7 @@ def execute_async(self, code: str, language: str = "python", timeout: int = 30)
159159
try:
160160
error_data = response.json()
161161
error_detail = error_data.get("detail", error_detail)
162-
except:
162+
except (ValueError, requests.exceptions.JSONDecodeError):
163163
error_detail = response.text
164164

165165
raise ExecutionError(f"Async execution failed: {error_detail}")
@@ -250,7 +250,7 @@ def is_session_active(self) -> bool:
250250
try:
251251
response = self._session.get(f"{self.base_url}/sessions/{self.session_id}")
252252
return response.status_code == 200
253-
except:
253+
except requests.exceptions.RequestException:
254254
return False
255255

256256
def list_sessions(self) -> Dict[str, Any]:
@@ -331,7 +331,7 @@ def _create_session(self) -> str:
331331
try:
332332
error_data = response.json()
333333
error_detail = error_data.get("detail", error_detail)
334-
except:
334+
except (ValueError, requests.exceptions.JSONDecodeError):
335335
error_detail = response.text
336336

337337
raise SessionError(f"Failed to create session: {error_detail}")

api/rest_server.py

Lines changed: 68 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
"""REST API server for CodeRunner - InstaVM compatible interface"""
22

3-
from fastapi import FastAPI, HTTPException, Depends
3+
from fastapi import FastAPI, HTTPException, Depends, BackgroundTasks
44
from fastapi.middleware.cors import CORSMiddleware
55
from fastapi.responses import JSONResponse
66
import logging
@@ -69,7 +69,7 @@ async def health_check():
6969
session_count = len(session_manager.sessions)
7070

7171
# Check kernel pool
72-
from server import kernel_pool
72+
from core.kernel_pool import kernel_pool
7373
kernel_status = {
7474
"total_kernels": len(kernel_pool.kernels),
7575
"available_kernels": len([k for k in kernel_pool.kernels.values() if k.is_available()]),
@@ -165,13 +165,70 @@ async def execute_command(request: CommandRequest):
165165
raise HTTPException(status_code=500, detail=f"Internal server error: {str(e)}")
166166

167167

168+
async def _execute_task_background(task_id: str, command: str, language: str, session_id: str):
169+
"""Background task to execute code asynchronously"""
170+
try:
171+
# Update status to running
172+
async_tasks[task_id]["status"] = "running"
173+
174+
# Get or create session
175+
if not session_id:
176+
session_id = await session_manager.create_session(language)
177+
elif not await session_manager.get_session(session_id):
178+
async_tasks[task_id]["status"] = "failed"
179+
async_tasks[task_id]["error"] = "Session not found"
180+
return
181+
182+
# Execute command
183+
result = await session_manager.execute_in_session(session_id, command)
184+
185+
# Extract output from result
186+
stdout = ""
187+
stderr = ""
188+
189+
if isinstance(result, dict):
190+
if "stdout" in result:
191+
stdout = result["stdout"]
192+
elif "output" in result:
193+
stdout = result["output"]
194+
else:
195+
stdout = str(result)
196+
197+
if "stderr" in result:
198+
stderr = result["stderr"]
199+
elif "error" in result and result.get("error"):
200+
stderr = str(result["error"])
201+
else:
202+
stdout = str(result)
203+
204+
# Update task with results
205+
async_tasks[task_id].update({
206+
"status": "completed",
207+
"result": {
208+
"stdout": stdout,
209+
"stderr": stderr,
210+
"session_id": session_id
211+
},
212+
"completed_at": time.time()
213+
})
214+
215+
except Exception as e:
216+
logger.error(f"Background task {task_id} failed: {e}")
217+
async_tasks[task_id].update({
218+
"status": "failed",
219+
"error": str(e),
220+
"completed_at": time.time()
221+
})
222+
223+
168224
@app.post("/execute_async", response_model=AsyncExecutionResponse)
169-
async def execute_command_async(request: CommandRequest):
225+
async def execute_command_async(request: CommandRequest, background_tasks: BackgroundTasks):
170226
"""
171227
Execute command asynchronously - InstaVM compatible interface
172228
173229
Args:
174230
request: Command execution request
231+
background_tasks: FastAPI background tasks
175232
176233
Returns:
177234
Task ID for checking execution status
@@ -199,9 +256,14 @@ async def execute_command_async(request: CommandRequest):
199256
"error": None
200257
}
201258

202-
# TODO: Implement actual async execution with background tasks
203-
# For now, just return the task ID
204-
# In a full implementation, this would use Celery or similar
259+
# Add background task for execution
260+
background_tasks.add_task(
261+
_execute_task_background,
262+
task_id,
263+
request.command,
264+
request.language,
265+
request.session_id
266+
)
205267

206268
return AsyncExecutionResponse(task_id=task_id, status="queued")
207269

container_manager.py

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -61,11 +61,11 @@ def is_healthy(cls) -> bool:
6161
"""
6262
try:
6363
response = requests.get(
64-
f"http://localhost:{cls.REST_PORT}/health",
64+
f"http://localhost:{cls.MCP_PORT}/api/health",
6565
timeout=3
6666
)
6767
return response.status_code == 200
68-
except:
68+
except requests.exceptions.RequestException:
6969
return False
7070

7171
@classmethod
@@ -238,7 +238,7 @@ def _show_container_logs(cls):
238238
print(result.stdout)
239239
if result.stderr:
240240
print("STDERR:", result.stderr)
241-
except:
241+
except subprocess.CalledProcessError:
242242
print(" Could not retrieve container logs")
243243

244244
@classmethod
@@ -366,7 +366,6 @@ def create_isolated_container(cls) -> Tuple[str, Dict[str, int]]:
366366
result = subprocess.run([
367367
"docker", "run", "-d",
368368
"--name", container_name,
369-
"-p", f"{ports['rest']}:8223",
370369
"-p", f"{ports['mcp']}:8222",
371370
"-p", f"{ports['jupyter']}:8888",
372371
"-p", f"{ports['playwright']}:3000",
@@ -377,7 +376,7 @@ def create_isolated_container(cls) -> Tuple[str, Dict[str, int]]:
377376
container_id = result.stdout.strip()
378377

379378
# Wait for container to be healthy
380-
cls._wait_for_isolated_health(ports['rest'])
379+
cls._wait_for_isolated_health(ports['mcp'])
381380

382381
logger.info(f"Created isolated container {container_id} ({container_name})")
383382
return container_id, ports
@@ -418,24 +417,24 @@ def is_port_available(port: int) -> bool:
418417
return ports
419418

420419
@classmethod
421-
def _wait_for_isolated_health(cls, rest_port: int, timeout: int = 120):
420+
def _wait_for_isolated_health(cls, mcp_port: int, timeout: int = 120):
422421
"""Wait for isolated container to be healthy"""
423422
start_time = time.time()
424423

425424
while time.time() - start_time < timeout:
426425
try:
427426
response = requests.get(
428-
f"http://localhost:{rest_port}/health",
427+
f"http://localhost:{mcp_port}/api/health",
429428
timeout=3
430429
)
431430
if response.status_code == 200:
432431
return
433-
except:
432+
except requests.exceptions.RequestException:
434433
pass
435434

436435
time.sleep(2)
437436

438-
raise TimeoutError(f"Isolated container on port {rest_port} failed to become healthy")
437+
raise TimeoutError(f"Isolated container on port {mcp_port} failed to become healthy")
439438

440439
@classmethod
441440
def remove_isolated_container(cls, container_id: str) -> bool:

0 commit comments

Comments
 (0)