Skip to content

[framework, gateway] fix: settle cancelled session creation - #241

Open
tongyx361 wants to merge 1 commit into
verl-project:mainfrom
tongyx361:pr/session-version-routing
Open

tongyx361 wants to merge 1 commit into
verl-project:mainfrom
tongyx361:pr/session-version-routing

Conversation

@tongyx361

@tongyx361 tongyx361 commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Cancelling a Gateway session creation can leave the remote actor session alive after its manager owner is removed. Wait for the create result and abort only the session that call successfully created, even when cancellation repeats. This is a self-contained lifecycle bug fix and needs no separate issue.

Changes

  • Shield remote creation and finish owned cancellation cleanup before propagating cancellation.
  • Retain the manager owner when abort fails so an explicit retry remains possible.
  • Reject duplicate session creation before changing ownership or load counts, and release a manager slot at most once for concurrent closes.
  • Preserve the original runner exception when Gateway abort also fails.

Route binding, route release and model-version semantics are implemented separately in the dependent #244. This PR adds no backend route hooks or custom release exceptions.

Validation

Local Python 3.12, with this checkout and its pinned public verl commit 2bc6ac46043e31855cea194236ff7548c91817c8 explicitly selected: 226 CPU tests passed, 1 deselected, covering remote creation, repeated cancellation, retained abort ownership, duplicate IDs, concurrent closes and runner-error preservation. All-file pre-commit and git diff --check passed. No accelerator-training claim is made.

python -m pytest -q \
  tests/uni_agent/framework/test_generate_sequences_on_cpu.py \
  tests/uni_agent/gateway/test_gateway_actor_on_cpu.py \
  tests/uni_agent/gateway/test_gateway_manager_on_cpu.py \
  tests/uni_agent/gateway/test_session_multiple_chains_on_cpu.py \
  -m 'cpu and level0'
pre-commit run --all-files --show-diff-on-failure
git diff --check

Compatibility

Public call signatures and ordinary backend behavior are unchanged. A cancelled create may wait for its remote result and owned abort rather than returning while that actor call is still running. IDs already owned by this manager are rejected before another create is dispatched.

Checklist

  • Focused fix; the Summary explains why no separate issue is needed.
  • The title follows the owning-layer format.
  • Regression tests cover the behavior and validation limits are stated.
  • Lifecycle behavior is documented in method docstrings; no new public API or configuration is added.
  • Compatibility impact is documented.
  • No credentials or private runtime data are included.
  • pre-commit run --all-files --show-diff-on-failure passes.

Summary by CodeRabbit

  • Bug Fixes
    • Improved session cleanup when agent runs fail: cleanup errors no longer replace the original run error.
    • Improved handling of cancelled or duplicate session creation and failed cleanup attempts, helping prevent session ownership and load tracking from becoming inconsistent.
    • Ensured concurrent session finalization and abort operations release load tracking only once.

@codecov-commenter

codecov-commenter commented Oct 2, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 97.14286% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
uni_agent/gateway/manager.py 96.77% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@tongyx361
tongyx361 force-pushed the pr/session-version-routing branch 2 times, most recently from 6e33238 to 62a0e60 Compare October 8, 2026 12:24
@tongyx361 tongyx361 changed the title [framework, gateway] feat: bind session routes to model versions [framework, gateway] fix: preserve sessions and trajectories during route cleanup Oct 8, 2026
@tongyx361
tongyx361 force-pushed the pr/session-version-routing branch from 3a1fcb6 to e4290f4 Compare October 14, 2026 11:29
@coderabbitai

coderabbitai Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

📝 Walkthrough

Gateway session creation now handles duplicate IDs and caller cancellation while remote creation is pending. Finalize and abort release routing state through a shared operation that decrements the load count only when it removes a route. Runner failure handling preserves the original runner error when session abort also fails. New tests cover these lifecycle and error-handling cases.

Priority: ➖ Normal

Merge Risk: 🔵 Low · up to e4290

A cancellation followed by an abort failure can leave a session and its gateway slot allocated. This narrow failure path needs an explicit cleanup retry or shutdown cleanup.

Security architecture risk: 🔵 Low · up to e4290

The changes strengthen session cleanup and preserve initiating errors without adding a demonstrated access or privilege expansion. A remaining trade-off is that cancellation can wait indefinitely for stalled remote cleanup, potentially delaying an entire rollout batch.

Retained concerns

  • Low · reliability · inferred: Cancelled creation now waits through repeated cancellation until remote creation and subsequent abort settle, without a deadline or alternate recovery owner. If either remote operation remains unresolved, the episode cannot terminate and the enclosing rollout gathers can remain blocked. This is an introduced failure-containment trade-off, not a verified attacker-triggerable denial of service. Actor creation is synchronous once dispatched, which reduces ordinary-path risk but does not bound remote-call completion.
Security review details

Security Blast Radius

  • inferred — The changed state is session routing and per-actor load estimation within an owned gateway pool. A stalled cancellation-settlement operation can also delay its enclosing rollout batch. The inspected code does not establish a newly reachable cross-tenant or privileged attack path.

Trust Boundaries and Controls

  • inferred — The lifecycle change preserves the observed parent-owned session identity and existing runner dispatch authority. Tracing fields to prompts and runner classes to configuration narrows the supplied input-origin gap, but does not establish who controls upstream prompts, configuration or deployment-level actor access.

Resilience and Maintainability Implications

  • inferred — Ambiguous distributed outcomes remain a pre-existing recovery limitation: actor creation inserts state before returning, while a failed create result causes the manager to discard its route; close-result failures retain the route even if actor removal already occurred. Production occurrence of result loss was not established. Explicit abort retry can reconcile an already-removed actor session, but the inspected framework does not automatically retry failed cleanup.

Hardening Proposals

  • proposed — Define a bounded cancellation-settlement policy that transfers unresolved cleanup to an explicit recovery owner without discarding session identity or prematurely releasing the route. This would preserve the new ownership guarantees while allowing parent cancellation to terminate predictably.
- [ ] Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@tongyx361 tongyx361 changed the title [framework, gateway] fix: preserve sessions and trajectories during route cleanup [framework, gateway] fix: settle cancelled session creation Oct 14, 2026

@coderabbitai coderabbitai 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.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository: verl-project/uni-agent/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 70b7dcd3-41b6-4193-bee1-caaca7f58330
📥 Commits

Reviewing files that changed from the base of the PR and between 31a499e and e4290f4.

📒 Files selected for processing: 4
  • tests/uni_agent/framework/test_generate_sequences_on_cpu.py
  • tests/uni_agent/gateway/test_gateway_manager_on_cpu.py
  • uni_agent/framework/framework.py
  • uni_agent/gateway/manager.py

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 8 remain after this review.

Comment on lines +117 to +121
try:
await self.abort_session(session_id)
except (Exception, asyncio.CancelledError):
# A failed abort leaves this owner available for an explicit retry.
logger.exception("session %s: abort failed after cancelled create", session_id)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

🔎 Supported by static analysis

🏁 Script executed:

#!/bin/bash
rg -nP -C8 'gateway_manager\.(create_session|abort_session)\s*\(' uni_agent/framework

Repository: verl-project/uni-agent

Length of output: 3593


🏁 Script executed:

set -o pipefail
printf '%s\n' '--- framework lifecycle ---'
nl -ba uni_agent/framework/framework.py | sed -n '770,885p'
printf '%s\n' '--- manager implementation ---'
nl -ba uni_agent/gateway/manager.py | sed -n '1,190p'
printf '%s\n' '--- relevant diff ---'
git diff --no-ext-diff --unified=25 31a499eb2e4b16bb5fdc609ddf444b9a3b5d11f3 e4290f4715268faf471ad301a43405336b72c3d0 -- uni_agent/framework/framework.py uni_agent/gateway/manager.py

Repository: verl-project/uni-agent

Length of output: 25285


🏁 Script executed:

set -o pipefail
nl -ba uni_agent/framework/framework.py | sed -n '780,885p'
nl -ba uni_agent/gateway/manager.py | sed -n '1,190p'
git diff --no-ext-diff --unified=20 31a499eb2e4b16bb5fdc609ddf444b9a3b5d11f3 e4290f4715268faf471ad301a43405336b72c3d0 -- uni_agent/framework/framework.py uni_agent/gateway/manager.py

Repository: verl-project/uni-agent

Length of output: 23616


🏁 Script executed:

set -o pipefail
rg -n -F --glob '*.py' -e 'async def abort_session' -e 'def abort_session' -e 'async def shutdown' -e 'def shutdown' uni_agent/gateway
printf '%s\n' '--- gateway actor lifecycle ---'
nl -ba uni_agent/gateway/gateway.py | sed -n '1,260p'

Repository: verl-project/uni-agent

Length of output: 13784


🏁 Script executed:

nl -ba uni_agent/gateway/gateway.py | sed -n '252,310p'

Repository: verl-project/uni-agent

Length of output: 2530


Retry failed cleanup for cancelled creates.

GatewayManager.create_session calls _abort_cancelled_create when creation is cancelled, but that helper calls abort_session only once. If the remote abort fails, the route and load slot remain allocated. The actor also keeps the session because it removes it only after session.abort() succeeds. The Framework's later cancellation handler cannot handle cancellation of the preceding create_session await. Add a bounded, cancellation-safe retry, or track the session for shutdown cleanup.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants