agentscope-ai / agentscope-ai/agentscope
[Bug]: Concurrent PATCH sessions causes data loss
- Lingua principale
- Python
- Stelle
- 31.5k
- Fork
- 3.5k
- Merge medio
- 1g 23h
- PR unite (30g)
- 95
Descrizione
## Description
The `PATCH /sessions/{session_id}` endpoint implements a read-modify-write pattern without any concurrency control:
```python
# agentscope/app/_router/_session.py
async def update_session(...):
# Step 1: Read existing session
existing = await storage.get_session(user_id, agent_id, session_id)
# Step 2: Merge PATCH fields in memory
config_updates = body.model_dump(exclude_unset=True, exclude={'permission_mode'})
merged_config = {**existing.config.model_dump(mode="json"), **config_updates}
# Step 3: Write back
return await storage.upsert_session(
config=SessionConfig.model_validate(merged_config),
...
)
```
## Problem
When two PATCH requests arrive concurrently (e.g., one updating `name`, another updating `permission_mode`), both read the same old session state, merge their changes, and write back. The second write overwrites the first one's changes.
Example timeline:
```
T1: PATCH-1 reads config -> {name: "新会话", mode: "ask"}
T2: PATCH-2 reads config -> {name: "新会话", mode: "ask"} <- reads stale data
T3: PATCH-1 writes config -> {name: "改名", mode: "ask"}
T4: PATCH-2 writes config -> {name: "新会话", mode: "bypass"} <- overwrites PATCH-1's name
```
## Impact
- Data loss for session config updates
- Affects all storage backends (Redis, SQLite, local files, etc.) — this is NOT storage-specific
- The bug is at the router layer, not the storage layer
- Triggers when clients send concurrent PATCH requests (common in first-message scenarios)
- Critical for distributed deployments (multi-process, multi-instance)
## Root Cause
The router combines multiple storage primitives (get_session + merge + upsert_session) without atomicity guarantee. Storage backends only guarantee single-operation atomicity, not the atomicity of combined business operations.
This is an **application-layer bug** that cannot be solved by changing storage backends.
## Reproduction
1. Create a session
2. Send two concurrent PATCH requests:
```bash
curl -X PATCH ".../sessions/{id}?agent_id=..." -d '{"name":"test1"}' &
curl -X PATCH ".../sessions/{id}?agent_id=..." -d '{"permission_mode":"bypass"}' &
wait
```
3. Check session config — `name` may still be the old value
## Environment
- AgentScope version: 2.0.4.post1 (also affects latest main branch)
- Python version: 3.12
- Affects all storage backends
## Suggested Solutions
### Option 1: Optimistic Locking (Recommended)
Add a `version` field to `SessionRecord` and use CAS (compare-and-swap) during update:
```python
class SessionRecord(BaseModel):
version: int = 1
...
# StorageBase interface
async def cas_session(
self, session_id: str, expected_version: int, new_config: SessionConfig
) -> SessionRecord | None:
"""Compare-and-swap. Returns None if version mismatch."""
# Router
existing = await storage.get_session(...)
updated = await storage.cas_session(
session_id, expected_version=existing.version, new_config=merged_config
)
if updated is None:
raise HTTPException(409, "Session modified, please retry")
```
**Pros**: Storage-agnostic, no external lock service needed, works across processes/instances
**Implementation notes**:
- Redis: use WATCH/MULTI/EXEC for CAS
- SQLite: use `UPDATE ... WHERE version = ?` and check rowcount
- Each storage backend implements `cas_session` accordingly
### Option 2: Distributed Lock
Use Redis lock or database advisory lock keyed by session_id:
```python
# Router
async with distributed_lock(f"session:{session_id}"):
existing = await storage.get_session(...)
# merge and update
```
**Pros**: Explicit mutual exclusion, predictable behavior
**Cons**: Requires lock service (Redis), adds latency, needs timeout handling
Thanks!
Guida per i contributori
Apri la guida per i contributori
Valutazione
Questa issue non è ancora stata valutata.