Skip to content

Fix async event loop and task lifecycle - #1507

Merged
Yunnglin merged 17 commits into
mainfrom
agent/fix-sandbox-event-loop-1504
Jul 24, 2026
Merged

Yunnglin merged 17 commits into
mainfrom
agent/fix-sandbox-event-loop-1504

Conversation

@Yunnglin

@Yunnglin Yunnglin commented Jul 23, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Fix the async lifecycle issues found while reproducing #1504:

  • give framework-owned event loops one graceful shutdown protocol
  • isolate perf benchmark completion state and propagate producer/consumer errors
  • bind external bridge instances to their real owner loop and await streaming task cancellation
  • structure Toolathlon relay concurrency around one shared HTTP client and supervised tasks
  • retain the existing explicit ModelAPI.aclose() / Model.aclose() lifecycle for caller-managed loops

Root cause

The original failure came from mismatched resource and event-loop lifetimes. A cached Volcengine sandbox manager and its async transports were created on a worker-owned loop, but that loop was closed after the sample. Later samples reused resources still bound to the closed loop.

The same ownership class appeared in adjacent paths:

  • loop shutdown closed the loop without draining tasks, async generators, or the default executor
  • perf benchmarks shared a module-level completion event and swallowed strategy task failures
  • the external bridge keyed instances by id(loop) and canceled streaming generation without awaiting it
  • Toolathlon detached request tasks and created an HTTP connection pool per request

Changes

Owned event loops

  • run close callbacks on the owner loop for both external-thread and owner-thread stop()
  • cancel and await residual tasks before closing
  • run shutdown_asyncgens() and shutdown_default_executor()
  • keep shutdown idempotent and close the loop only from its owner thread
  • reuse the same graceful close helper for perf's signal/uvloop-compatible main loop

Perf

  • replace the global completion event with one event per benchmark
  • supervise producer, queue drain, and metrics consumer as one pipeline
  • cancel sibling tasks immediately when producer, consumer, or strategy work fails
  • propagate original exceptions instead of SystemExit
  • raise FileExistsError for an existing result database
  • make AioHttpClient.__aenter__() return self and keep close idempotent

External bridge

  • key the registry by real event-loop objects and retain the owner loop
  • enforce shutdown on the owner loop and remove the exact owner entry
  • await canceled OpenAI, Anthropic, and Gemini generation tasks
  • preserve both runner callback shutdown and explicit shutdown

Toolathlon

  • share one httpx.AsyncClient for the WebSocket relay lifetime
  • supervise receive, request processing, heartbeat, and active request tasks
  • propagate request/WebSocket/heartbeat failures
  • complete queue bookkeeping in finally and await every canceled task before exit

Public behavior

  • run_perf_benchmark() now raises the original Python exception instead of exiting the process
  • the internal data_process_completed_event export is removed
  • model, sandbox, MCP, and agent environment resource boundaries remain unchanged
  • leaf asyncio.run() calls in synchronous CLI entry points remain unchanged

Validation

  • core/model/perf/Toolathlon/sandbox/environment: 122 passed, 10 skipped
  • external-agent suite: 66 passed, 8 skipped
  • workload trace local HTTP integration: 8 passed
  • CLI smoke: 1 passed
  • conda run -n eval make lint
  • SandboxFusion HumanEval canonical 10: mean_acc=1.0, mean_acc_pass@1=1.0
  • latest SandboxFusion log contains no pending-task, unclosed-session, or event-loop-closed warning

Fixes #1504

@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@Yunnglin Yunnglin closed this Jul 23, 2026
@Yunnglin Yunnglin reopened this Jul 23, 2026
@Yunnglin Yunnglin closed this Jul 23, 2026
@Yunnglin Yunnglin reopened this Jul 23, 2026
@Yunnglin Yunnglin changed the title Fix sandbox manager event loop lifecycle Fix async event loop and task lifecycle Jul 23, 2026
@Yunnglin Yunnglin added the qoder-review Add to a PR to trigger Qoder code review label Jul 23, 2026
@Yunnglin
Yunnglin marked this pull request as ready for review July 23, 2026 08:50
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@qoderai qoderai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 Review Summary

This PR introduces a coherent set of async lifecycle improvements across perf benchmarks, model async clients, sandbox service, external bridge streaming, and Toolathlon relays. The intent is clear: align event-loop ownership with resource lifetimes, supervise producer/consumer pipelines, and make shutdown/cancellation predictable. The changes are internally consistent and backed by targeted tests.

🛡️ Key Risks & Issues

  • Perf benchmark pipeline and strategies: The new run_benchmark_pipeline flow (evalscope/perf/core/metrics_consumer.py:138) correctly coordinates producer, queue drain, and consumer tasks, with explicit failure checks and cleanup via cancel_and_wait. Closed-loop and multi-turn strategies now structure task creation and cancellation so in-flight work is cleaned up on errors. I did not identify race conditions or leaks here; the main behavioral change is that early consumer exit or producer failure surfaces as a runtime error instead of being silently swallowed.
  • Async model client lifecycle: The loop-bound client pool (evalscope/models/utils/async_client.py:17) and ModelAPI.aclose() hooks make ownership explicit and ensure async clients are closed on their owner loops. The runtime error when attempting cleanup after the owner loop is stopped is intentional and encourages callers to call Model.aclose() before shutting down loops. The logic for owned vs external loops is sound, and new tests exercise the core invariants.
  • External bridge streaming and Toolathlon relay: Streaming handlers in the bridge now await generation task cancellation via cancel_and_wait (evalscope/agent/external/bridge/server.py:433, 464, 688, 771), reducing the risk of pending tasks or partially torn-down sessions after disconnects. Toolathlon’s run_ws_proxy has been restructured around a shared httpx.AsyncClient, supervised request processing, heartbeat monitoring, and coordinated cancellation (evalscope/benchmarks/toolathlon/client.py:357). The error and timeout paths look robust, and the tests cover several important scenarios.
  • Event-loop ownership and shutdown semantics: AsyncioLoopThread, AsyncioLoopRunner, and ModelProxyServer now enforce that close callbacks and bridge shutdown happen on the owning loop. This is a stricter, safer contract that prevents cleanup on the wrong loop. The tests verify that callbacks run on the expected loops, async generators are finalized, pending tasks are cancelled, and default executors are drained.

I did not find logic bugs, security risks, or severe performance issues that would block this PR.

🧪 Verification Advice

  • Run the documented perf and CLI smoke tests on environments that mimic your production setup (including uvloop or custom loop runners) to confirm there are no regressions in long-running sweeps or multi-benchmark runs.
  • Exercise scenarios where benchmarks are run sequentially and concurrently, watching for warnings about pending tasks, closed event loops, or unclosed HTTP sessions; the new lifecycle code should keep logs clean.
  • Verify that external bridges and Toolathlon relays behave correctly under mid-stream failures (remote disconnect, heartbeat timeouts, websocket send errors) and that no background tasks are left running after the run completes.
  • In integrations that supply their own event loop (e.g., web frameworks, notebooks), explicitly call Model.aclose() / ModelAPI.aclose() before shutting down the loop and confirm that cleanup completes without errors.

💡 Thoughts & Suggestions

  • Consider adding a small number of integration-style tests that run a full evaluation involving perf, async model clients, sandbox, external bridges, and Toolathlon together, then assert that there are no remaining tasks or open loops/sessions after completion. This would reinforce the async lifecycle guarantees this PR is aiming for.
  • It may be useful to document, at a high level, the new expectation that callers should invoke Model.aclose() before stopping event loops that host async clients, especially for users embedding evalscope into their own async runtimes.
  • Overall, this is a substantial and well-structured improvement to the async lifecycle story in evalscope, and the added tests significantly increase confidence in the changes.

🤖 Generated by Qoder • View workflow run

@Yunnglin
Yunnglin force-pushed the agent/fix-sandbox-event-loop-1504 branch from 6b55a4f to 1e5e588 Compare July 24, 2026 05:25
@Yunnglin
Yunnglin merged commit 0e383e5 into main Jul 24, 2026
3 checks passed
@Yunnglin
Yunnglin deleted the agent/fix-sandbox-event-loop-1504 branch September 10, 2026 07:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

qoder-review Add to a PR to trigger Qoder code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

【bug】evalscope从1.5.1升级到1.9.0,volcengine sandbox环境的评测得分异常

1 participant