Skip to content

fix(subagents): preserve batch stop cleanup across cancellation - #6552

Merged
WillemJiang merged 2 commits into
bytedance:mainfrom
FanouZeng-TT:fix/batch-stop-cancellation-retry
Oct 9, 2026
Merged

WillemJiang merged 2 commits into
bytedance:mainfrom
FanouZeng-TT:fix/batch-stop-cancellation-retry

Conversation

@FanouZeng-TT

Copy link
Copy Markdown
Contributor

Why

The durable batch stop added by #6517 directly awaited the poller and item tasks after cancelling them. asyncio.wait_for() in the Gateway cancels its stop() caller at the shutdown deadline; that cancellation propagates through gather() into the service-owned tasks while they are already draining their own cancellation cleanup.

The result is not just a slow shutdown. Caller cancellation can mutate or terminate the only cleanup operation, so a later stop() does not join the original drain, item bookkeeping can remain owned after the child has been torn down, and a restart has no reliable lifecycle boundary. The existing cancellation-retry regression also hung on current main because the test waited for cleanup that its own cancelled waiter had re-cancelled.

What changed

  • Give each stop generation one service-owned cleanup task.
  • Shield that cleanup from every caller, including the Gateway's bounded wait_for().
  • Make concurrent and retrying stop() callers join the same cleanup task rather than issuing competing cancellations.
  • Keep start() fenced until the shared cleanup completes successfully; cleanup failures remain visible to later lifecycle calls.
  • Add deterministic coverage for concurrent stops, cancelled-stop retry, cleanup failure, the direct __new__ shutdown fixture, and the real Gateway timeout boundary.

This keeps the Gateway's five-second latency bound: the shutdown hook still returns after its deadline, while the service-owned cleanup continues under explicit ownership instead of being corrupted by that timeout.

Surface area

  • Frontend UI — page / component / setting / interaction under frontend/
  • Backend API — endpoint / SSE event / request-response shape under backend/app
  • Agents / LangGraph — durable subagent batch worker lifecycle only; no prompt or graph change
    • Prompt-layer self-check: no prompt text or trust-channel change.
  • Sandbox — docker/ or sandboxed execution
  • Skills — change under skills/
  • Dependencies — new/upgraded entry in backend/pyproject.toml or frontend/package.json
  • Default behavior change — Gateway timeout/caller cancellation no longer cancels service-owned batch cleanup
  • Docs / tests / CI only — runtime behavior changes

Screenshots / Recording

N/A — backend lifecycle fix.

Bug fix verification

Tests:

  • backend/tests/test_subagent_batch_service.py::test_cancelled_batch_stop_allows_successful_retry
  • backend/tests/test_subagent_batch_service.py::test_concurrent_batch_stops_share_one_cleanup
  • backend/tests/test_subagent_batch_service.py::test_batch_stop_cleanup_failure_is_reported_to_retry
  • backend/tests/test_subagent_batch_shutdown_timeout.py::test_gateway_shutdown_timeout_does_not_cancel_batch_cleanup

Red on current main 5ecc1c2a: yes. The pre-existing cancelled-stop test does not complete under an external deadline. faulthandler_timeout=10 shows the event loop still waiting after cancellation. The Gateway regression likewise fails before the fix because its timeout cancels the owned cleanup instead of leaving a joinable cleanup generation.

Green on this branch: yes. The focused lifecycle matrix is 30 passed, including the existing runtime-owned cancellation tests and the __new__ shutdown fixture.

Collision audit immediately before submission:

Validation

Python 3.12.12, locked backend dependencies:

  • PYTHONPATH=. uv run pytest tests/test_subagent_batch_service.py tests/test_subagent_batch_shutdown_timeout.py tests/test_subagent_runtime.py -q — 30 passed
  • make lint — Ruff check clean; 1,895 files formatted
  • make test-blocking-io — 235 passed
  • make test — all four shards passed:
    • 6,718 passed / 108 skipped
    • 6,713 passed / 118 skipped
    • 6,720 passed / 109 skipped
    • 6,712 passed / 117 skipped
  • git diff --check — clean

The first local full-suite attempt exposed an incomplete environment-only readabilipy Node dependency install. Its unrelated link-extraction file reproduced 22 failures alone. Restoring the lock-identical package's missing Node files made that file 48/48 green; the final two complete four-shard runs then passed.

python3 scripts/check_agent_guidance.py still reports the unchanged inherited backend/packages/harness/deerflow/agents/middlewares/AGENTS.md chain at 98,404 bytes versus the 98,304-byte limit. This PR changes no guidance file; make lint, which checks the committed changed-file path policy, passes.

AI assistance

Tool(s) used: Claude Code

How you used it: I used Claude Code to trace the asyncio cancellation propagation, reproduce the current-main hang, implement the service-owned cleanup generation and deterministic regressions, audit overlapping PRs, and run the focused and complete validation matrix. I reviewed the final three-file diff and validation evidence.

  • I've read and understand every line of this change and take responsibility for it — it's not unreviewed AI output.

@github-actions github-actions Bot added area:agents Agents, subagents, graph wiring, prompts, langgraph.json needs-validation Touches front/back contract surface; needs real-path validation risk:high High risk: backend API, agents, sandbox, auth, deps, CI size/M PR changes 100-300 lines labels Oct 9, 2026
Comment on lines +81 to +83
cleanup_task = getattr(self, "_stop_cleanup_task", None)
if cleanup_task is not None:
cleanup_task.result()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P2] Reset the completed cleanup task before restarting

After start → stop → start → stop, the final stop() reuses the first generation's completed _stop_cleanup_task. It returns without setting _stop, requesting cancellation, or draining the restarted poller and item tasks, so owned work can outlive the runtime's shutdown.

I reproduced this with an event-based regression: the restarted poller is drained on base 5ecc1c2a, but remains pending on PR head 8fc403b0. All 30 existing related unit tests pass; the restart tests currently call the final stop() without asserting that the new poller stopped.

Clear _stop_cleanup_task after cleanup_task.result() succeeds, before starting the new generation, and assert that the second stop terminates the restarted poller and clears ownership. That reset passes both the timeout-preservation and restart probes in an isolated candidate implementation.

@github-actions github-actions Bot added area:docs Documentation and Markdown only size/L PR changes 300-700 lines and removed size/M PR changes 100-300 lines labels Oct 9, 2026
@WillemJiang WillemJiang added this to the 2.2.0 milestone Oct 9, 2026
@WillemJiang
WillemJiang merged commit 02ce9ab into bytedance:main Oct 9, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:agents Agents, subagents, graph wiring, prompts, langgraph.json area:docs Documentation and Markdown only needs-validation Touches front/back contract surface; needs real-path validation risk:high High risk: backend API, agents, sandbox, auth, deps, CI size/L PR changes 300-700 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants