Repository navigation
Conversation
🧪 Code Coverage (vs
|
| Base | PR | Delta | |
|---|---|---|---|
| src/a2a/server/agent_execution/active_task_registry.py | 97.01% | 97.26% | 🟢 +0.25% |
| Total | 93.06% | 93.06% | ⚪️ 0.00% |
Generated by coverage-comment.yml
mykytanetipa
left a comment
There was a problem hiding this comment.
Thank you for this contribution. Could you rebase this onto latest main? It's built on an older base (_lock is now threading.RLock, and get_or_create was restructured)
| # ActiveTask.start() on failure. That cleanup runs in a separate | ||
| # task and may not execute before another request checks the | ||
| # registry, leaving a zombie entry for a task that never started. | ||
| async with self._lock: |
There was a problem hiding this comment.
async lock doesn't make a lot of sense here for single sync pop with no await, you've likely kept it for consistency with codebase which already moved to threading.RLock.
d7941ab to
407a5fa
Compare
|
Rebased onto latest |
What changed
1. Remove registry entries synchronously when
ActiveTask.start()failsProblem:
ActiveTaskRegistry.get_or_create()(src/a2a/server/agent_execution/active_task_registry.py) inserts theActiveTaskinto_active_tasksbefore callingstart(). Ifstart()raises (e.g. the task is already in a terminal state), the only cleanup was the fire-and-forgetasyncio.create_task(self._remove_task(...))scheduled via_on_active_task_cleanupinsideActiveTask.start(). That cleanup runs asynchronously and may not execute before another request checks the registry, leaving a zombie entry for a task that never started — and a concurrentget_or_createcould return the half-started task.Fix (src/a2a/server/agent_execution/active_task_registry.py):
start()call inget_or_create()withtry/except; on any failure the entry is removed synchronously under_lockbefore the exception propagates.pop), so the fire-and-forget cleanup task scheduled byActiveTask.start()remains harmless if it runs later.Testing
./.venv/Scripts/python -m pytest tests/server/agent_execution/test_active_task_registry.py -q→ 6 passed (includes newtest_get_or_create_failed_start_removes_zombie_entry, which seeds a completed task sostart()raisesInvalidParamsError, then asserts the registry no longer contains the task)../.venv/Scripts/python -m pytest tests/server/agent_execution/ -q→ 73 passed../.venv/Scripts/python -m pytest tests/server/request_handlers/test_default_request_handler_v2.py -q→ 59 passed (registry consumers unaffected)../.venv/Scripts/python -m ruff checkon modified files: clean.start()the registry is now guaranteed not to retain the task.