[Bug] ModelClient: Missing error handling for JSON parsing and blocking I/O in async methods
- Dominant language
- Python
- Stars
- 485
- Forks
- 81
- Avg merge
- 16h 12m
- Merged PRs (30d)
- 8
Description
**Bug Description**
`ModelClient` in Python SDK has two major issues:
1. **Missing error handling for `json.loads()`**: The `parse_request_line()` and `parse_response_line()` methods call `json.loads(meta_json)` without try/catch. If the log file is partially written or corrupted, this will throw an unhandled `JSONDecodeError` exception.
2. **Synchronous file I/O in async methods**: All file operations (`read_last_request_line()`, `read_last_response_line()`, `wait_for_first_request()`, `_append_response()`) use synchronous `open()` and `readlines()` inside `async` methods. This blocks the asyncio event loop and defeats the purpose of using async/await.
**Steps to Reproduce**
1. Create a `ModelClient` instance
2. Simulate a corrupted log file with malformed JSON in the meta section:
```python
# Write a corrupted line to the log file
with open(log_file, 'w') as f:
f.write("__REQUEST_START__some_request__REQUEST_END__{invalid json}\n")
```
3. Call `await client.pop_request(1)`
4. Observe the unhandled `JSONDecodeError`
**Expected Behavior**
1. `json.loads()` should be wrapped in try/catch, and a meaningful error should be raised or logged when JSON parsing fails.
2. File I/O operations should use async libraries like `aiofiles` to avoid blocking the event loop.
**Actual Behavior**
1. `JSONDecodeError` is thrown without context, making it difficult to debug log file issues.
2. Async methods perform blocking I/O, which can cause performance issues in high-concurrency scenarios.
**Error Logs**
```
Traceback (most recent call last):
File "...", line X, in parse_request_line
meta = json.loads(meta_json)
^^^^^^^^^^^^^^^^^^^^^
json.decoder.JSONDecodeError: Expecting property name enclosed in double quotes: line 1 column 2 (char 1)
```
**Environment Information**
- **OS**: [e.g. Ubuntu 22.04, macOS 13.0, Windows 11]
- **Python Version**: [e.g. 3.11.5]
- **ROCK Version**: [e.g. 0.2.0]
- **Installation Method**: [e.g. pip install rl-rock, source installation with uv]
- **Docker Version**: [e.g. 24.0.6] (if using sandbox features)
- **Deployment Type**: [e.g. local, distributed, ray]
**ROCK Configuration**
- **Runtime Environment Type**: [e.g. uv, pip, conda]
- **Sandbox Image**: [e.g. python:3.11, custom image]
- **Resource Allocation**: [e.g. memory=8g, cpus=2.0]
**Component Affected**
- [ ] Sandbox
- [ ] Actions
- [ ] Deployments
- [x] SDK & API
- [ ] Envhub
- [ ] CLI
- [ ] Performance & Optimization
- [ ] Documentation & Examples
---
**Suggested Fix**
1. Add try/catch for JSON parsing:
```python
async def parse_request_line(self, line_content: str) -> tuple[str, dict]:
if SESSION_END_MARKER in line_content:
return SESSION_END_MARKER, {}
try:
meta_json = line_content.split(REQUEST_END_MARKER)[1]
request_json = line_content.split(REQUEST_END_MARKER)[0].split(REQUEST_START_MARKER)[1]
meta = json.loads(meta_json)
return request_json, meta
except (IndexError, json.JSONDecodeError) as e:
logger.error(f"Failed to parse request line: {line_content!r}, error: {e}")
raise ValueError(f"Invalid request line format: {e}") from e
```
2. Use `aiofiles` for async file I/O:
```python
import aiofiles
async def read_last_request_line(self) -> str:
async with aiofiles.open(self.log_file) as f:
lines = await f.readlines()
# ... rest of the logic
```
Or alternatively, use `asyncio.to_thread()` to wrap synchronous I/O:
```python
async def read_last_request_line(self) -> str:
def _sync_read():
with open(self.log_file) as f:
return f.readlines()
lines = await asyncio.to_thread(_sync_read)
# ... rest of the logic
```
Contributor guide
No contributing guide indexed for this repository
Research direction
Locate the ModelClient implementation and inspect parse_request_line(), parse_response_line(), read_last_request_line(), read_last_response_line(), wait_for_first_request(), and _append_response(). Reproduce the malformed-meta JSON case, then verify parsing failures have useful context and the listed async file operations no longer block the event loop.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- api, performance
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 45/100