fix(sdk/python): add lock timeouts + offload blocking requests fallback (#620) - #904
Conversation
…ck (Agent-Field#620) Slice 4 of Agent-Field#620: prevents indefinite hangs from contended locks and offloads the remaining blocking HTTP call in an async function. Lock timeouts: - New agentfield/lock_utils.py: timed_lock() context manager that acquires with a configurable timeout (default 30s, env var AGENTFIELD_LOCK_TIMEOUT_SECONDS) and raises LockTimeoutError with diagnostic info instead of hanging. - Applied to all lock sites in result_cache.py (11), cost_tracker.py (8), node_logs.py (7) — the 26 highest-contention acquisitions. Blocking request offload: - memory_events.py history() fallback: the blocking requests.get() in the async function's ImportError path is now offloaded to loop.run_in_executor() so it doesn't freeze the event loop. - Removed the ASYNC210 per-file-ignore for memory_events.py (resolved). Running-loop guard: - client.execute_sync() now emits a RuntimeWarning when called from within a running event loop, directing users to await execute() instead. Tests: 7 tests in test_lock_timeout.py covering timeout behaviour, reentrant locks, cross-thread contention, error attributes, and the execute_sync warning. 63 tests pass across the affected test surface. Part of Agent-Field#620.
Performance
✓ No regressions detected |
📊 Coverage gateThresholds from
✅ Gate passedNo surface regressed past the allowed threshold and the aggregate stayed above the floor. |
📐 Patch coverage gateThreshold: 80% on lines this PR touches vs
✅ Patch gate passedEvery surface whose lines were touched by this PR has patch coverage at or above the threshold. |
AbirAbbas
left a comment
AbirAbbas
left a comment
There was a problem hiding this comment.
Verified this locally on the CI-exact gates: ruff 0.15.22 clean and the full run_pytest.sh suite green on both 3.10 and 3.12 (1933 passed each), plus the websockets-compat matrix. Anti-tautology probes check out — patching timed_lock back to a bare acquire() makes the timeout test genuinely hang, and reverting client.py fails the running-loop-warning test, so those guards are real. Lock-site conversion is exact (11/8/7, zero bare with …_lock stragglers, remaining ones are async with on asyncio locks and correctly out of scope), and the RLock reentrancy in result_cache survives acquire(timeout=).
Three things inline, none of which I'd hold the merge for — the env-var parsing one is the one I'd fix first since it can fail import agentfield outright. One micro-nit not worth a thread: the tests/test_lock_timeout.py module docstring claims a memory_events.history() non-blocking test that doesn't exist (that change is guarded by the ASYNC210 ruff gate instead, which does fail on revert — I checked).
DEFAULT_LOCK_TIMEOUT was resolved with a bare float() at import time, so a
malformed value took down `import agentfield` altogether. The empty-string
case is the common one: `AGENTFIELD_LOCK_TIMEOUT_SECONDS=` in a compose
`env:` block makes float("") raise from __init__.py -> result_cache.py ->
lock_utils.py. A negative value imported fine but broke every lock op, since
lock.acquire(timeout=-5) raises ValueError.
Parsing now falls back to 30s for missing, empty, non-numeric, non-positive
and non-finite values, warning through the module logger for the cases that
look like a misconfiguration. Tests drive a fresh interpreter per value so
the import-time path is the one under test.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
On Python 3.11+ asyncio.TimeoutError is TimeoutError, so deriving from it made LockTimeoutError catchable by every `except asyncio.TimeoutError` up the stack. Agent.call wraps client.execute in asyncio.wait_for and that path goes through the result cache, so a real lock deadlock surfaced as "Execute call timed out" and the holder/wait diagnostics were lost. Deriving from RuntimeError instead keeps the message intact on all matrix versions. Nothing in the repo catches LockTimeoutError, so no call sites change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…string The __new__-based client only avoided real I/O by accident: execution died on a missing caller_agent_id attribute inside `except Exception: pass`. Give caller_agent_id a value and stub _submit_execution_sync with a sentinel, so the test asserts the RuntimeWarning and proves nothing was submitted — rather than depending on a crash that a class-level default would silence, turning the test into a live POST to localhost:8080 plus a polling loop. The module docstring also advertised a memory_events.history() test that was never written; say where that change is actually guarded instead. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AbirAbbas
left a comment
AbirAbbas
left a comment
There was a problem hiding this comment.
Re-approving at 37babd1 (my push dismissed the earlier stamp). All three review follow-ups are in as focused commits; independent verification probed them live: import succeeds with empty/garbage/negative env values (30s fallback, warning logged), LockTimeoutError no longer matches except asyncio.TimeoutError while keeping its diagnostic attributes, and the warning test runs with no control plane and no sockets. Full gates green locally and on CI (3.10–3.12 + websockets matrix). Note in the thread about the wait_for_result exception-type change — intended, but observable.
2 participants
Summary
Prevents indefinite hangs from contended locks and offloads the remaining blocking HTTP call inside an async function. This is slice 4 (final slice) of #620.
Type of change
What changed
Lock timeouts
New
agentfield/lock_utils.pyprovidestimed_lock()— a drop-in context manager replacement for barewith self._lock:that acquires with a configurable timeout (default 30s, env varAGENTFIELD_LOCK_TIMEOUT_SECONDS) and raisesLockTimeoutErrorwith diagnostic info instead of hanging forever.Applied to all lock sites in:
result_cache.py(11 sites)cost_tracker.py(8 sites)node_logs.py(7 sites, including module-level_follow_lock)Blocking request offload
memory_events.pyhistory()— the blockingrequests.get()fallback inside thisasync defis now offloaded toloop.run_in_executor()so it doesn't freeze the event loop. Removed theASYNC210per-file-ignore for this file (violation resolved).Running-loop guard
client.execute_sync()now emits aRuntimeWarningwhen called from within a running event loop, directing users toawait client.execute()instead. This catches the dangerous pattern before it causes a hang.Test plan
cd sdk/python && python -m pytest tests/test_lock_timeout.py -v(7 tests: timeout, reentrant, cross-thread, error attributes, warning)cd sdk/python && python -m pytest tests/test_result_cache.py tests/test_result_cache_deadlock.py tests/test_run_async.py tests/test_agent_core.py tests/test_client.py tests/test_client_execution_paths.py(63 passed)cd sdk/python && ruff check .cleanTest coverage
coverage-baseline.json— N/AChecklist
Related issues / PRs
Part of #620 (final slice)
Follows: