feat(llm): add overall timeout to ArticleFactChecker & add arXiv package#375
feat(llm): add overall timeout to ArticleFactChecker & add arXiv package#375seancoding-day merged 1 commit intoMigoXLab:devfrom
Conversation
ArticleFactChecker had no wall-clock timeout, risking unbounded execution on articles with many claims. Add asyncio.wait_for-based overall timeout (default 900s) with input validation, range clamping [30s, 7200s], and graceful error reporting on both normal and Jupyter fallback paths. Also add missing arxiv>=2.4.0 dependency to requirements/agent.txt. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces a wall-clock timeout mechanism for the ArticleFactChecker agent, including new configuration parameters, clamping logic, and a structured error response for timeout events. It also adds the arxiv dependency to the agent requirements. Feedback was provided to refactor duplicated timeout handling logic and to replace a magic number with a named constant to improve maintainability.
| async def _run_with_timeout() -> EvalDetail: | ||
| return await asyncio.wait_for( | ||
| cls._async_eval(input_data, start_time, output_dir), | ||
| timeout=timeout, | ||
| ) | ||
|
|
||
| try: | ||
| return asyncio.run(cls._async_eval(input_data, start_time, output_dir)) | ||
| return asyncio.run(_run_with_timeout()) | ||
| except asyncio.TimeoutError: | ||
| elapsed = time.time() - start_time | ||
| log.warning(f"ArticleFactChecker: overall timeout exceeded ({elapsed:.1f}s / {timeout:.0f}s limit)") | ||
| return cls._create_overall_timeout_result(elapsed, timeout) | ||
| except RuntimeError as e: | ||
| # Fallback when called inside an already-running event loop (e.g. Jupyter, tests) | ||
| if "cannot run" in str(e).lower() or "already running" in str(e).lower(): | ||
| import concurrent.futures | ||
| with concurrent.futures.ThreadPoolExecutor(max_workers=1) as pool: | ||
| future = pool.submit( | ||
| lambda: asyncio.run(cls._async_eval(input_data, start_time, output_dir)) | ||
| ) | ||
| return future.result() | ||
| future = pool.submit(lambda: asyncio.run(_run_with_timeout())) | ||
| try: | ||
| # Extra margin so asyncio.wait_for fires before this outer timeout | ||
| return future.result(timeout=timeout + 30) | ||
| except (asyncio.TimeoutError, concurrent.futures.TimeoutError): | ||
| elapsed = time.time() - start_time | ||
| log.warning( | ||
| f"ArticleFactChecker: overall timeout exceeded " | ||
| f"({elapsed:.1f}s / {timeout:.0f}s limit, fallback path)" | ||
| ) | ||
| return cls._create_overall_timeout_result(elapsed, timeout) | ||
| raise |
There was a problem hiding this comment.
There's some code duplication in the timeout handling logic within the except blocks. This can be refactored into a local helper function to improve maintainability.
Additionally, the 30 second margin added to the fallback timeout is a magic number. It would be better to define this as a local constant for clarity.
Here's a suggested refactoring that addresses both points:
async def _run_with_timeout() -> EvalDetail:
return await asyncio.wait_for(
cls._async_eval(input_data, start_time, output_dir),
timeout=timeout,
)
FALLBACK_TIMEOUT_MARGIN = 30 # seconds
def _handle_timeout_error(is_fallback: bool = False):
elapsed = time.time() - start_time
path_str = " (fallback path)" if is_fallback else ""
log.warning(
f"ArticleFactChecker: overall timeout exceeded "
f"({elapsed:.1f}s / {timeout:.0f}s limit{path_str})"
)
return cls._create_overall_timeout_result(elapsed, timeout)
try:
return asyncio.run(_run_with_timeout())
except asyncio.TimeoutError:
return _handle_timeout_error()
except RuntimeError as e:
# Fallback when called inside an already-running event loop (e.g. Jupyter, tests)
if "cannot run" in str(e).lower() or "already running" in str(e).lower():
import concurrent.futures
with concurrent.futures.ThreadPoolExecutor(max_workers=1) as pool:
future = pool.submit(lambda: asyncio.run(_run_with_timeout()))
try:
# Extra margin so asyncio.wait_for fires before this outer timeout
return future.result(timeout=timeout + FALLBACK_TIMEOUT_MARGIN)
except (asyncio.TimeoutError, concurrent.futures.TimeoutError):
return _handle_timeout_error(is_fallback=True)
raise
No description provided.