Skip to content

fix(agent): keep /stop responsive while a large-session compression commit is in flight - #98366

Open
liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-98351
Open

liuhao1024 wants to merge 1 commit into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-98351

Conversation

@liuhao1024

Copy link
Copy Markdown
Contributor

What does this PR do?

During a context-compression commit the worker retains the CompressionCommitFence lock from begin_commit() until finish_commit() — by design, an admitted SessionDB mutation is never abandoned mid-commit. The problem is that AIAgent.interrupt(hard_cancel=True) admitted the stop through the blocking cancel_before_commit(fence, event), which waits on that same fence lock for the whole commit to finish. On a large session (#98351, 1,500+ messages) the commit (archive/split/rotate) can run for minutes, so every explicit-stop producer was pinned behind it:

  • Desktop /stoptui_gateway._interrupt_session_turnrequest_hard_interrupt blocked on the fence, and it blocks while holding _pending_redirect_lock, so /new (which also interrupts) stopped answering too — the UI showed a permanent "Summarizing thread" overlay with a dead input box.
  • CLI Ctrl+C ran the same blocking admission inside a signal handler.
  • The gateway turn-timeout watchdog (_watch_gateway_turn_inactivity, explicitly designed to stay runnable when gateway asyncio is starved) called the same admission and deadlocked interrupting the very turn it was supposed to reap, so _reap_gateway_turn_processes never ran.

The fence already exposes the non-blocking try_cancel_before_commit() (plus the lock-free commit_in_flight observer) for exactly this reason — gateway session hygiene already uses the non-blocking form; the sync interrupt() path was the only remaining blocking caller.

This PR switches _admit_hard_cancel() to try_cancel_before_commit():

  • True (cancel won before the boundary) — same as before.
  • False (commit already finished) — same as before.
  • None (commit in flight) — the stop event is published immediately instead of waiting.

Semantics are unchanged where it matters: the commit is still never abandoned mid-mutation, the conversation thread still waits for the commit inside the compression call and observes the stop as soon as it returns, and the durable lock is released exactly as before. Every hard_interrupt producer only sets the flag — none of them consumes "commit settled", so the wait bought nothing while costing UI responsiveness. The blocking form remains as a fallback for fence objects that predate the non-blocking API.

Related Issue

Fixes #98351

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • run_agent.py_admit_hard_cancel() now admits through the non-blocking try_cancel_before_commit() and publishes the stop event without waiting behind an in-flight commit; the blocking cancel_before_commit remains as the legacy-fence fallback.
  • tests/agent/test_compression_concurrent_fork.pytest_hard_stop_waits_for_commit_already_admitted updated to test_hard_stop_returns_promptly_while_commit_in_flight: the stop must return while the commit is still blocked, and the already-admitted commit must still complete fully (compressed transcript lands, stop event set, durable compression lock released) — the end-state assertions from the old test are preserved.

How to Test

  1. python -m pytest tests/agent/test_compression_concurrent_fork.py tests/agent/test_compression_review_76354.py tests/agent/test_compression_interrupt_protection.py tests/agent/test_compression_stall_fallback_78981.py tests/run_agent/test_interrupt_propagation.py tests/run_agent/test_steer.py tests/agent/test_interrupt_compat.py -q128 passed (Observed result: 128 passed).
  2. python -m pytest tests/run_agent/ tests/test_tui_gateway_queue_on_busy.py tests/agent/test_subagent_lifecycle.py -q — 1952 passed; the 7 failures (test_nous_fallback_unavailable, test_run_agent Anthropic shared-client, 3× test_streaming Anthropic callbacks, 2× test_switch_model_reasoning_override) reproduce identically on a clean upstream/main checkout — pre-existing, unrelated to this change.
  3. The updated test itself is the regression proof: it blocks archive_and_compact mid-commit, calls hard_interrupt while the commit is in flight, and asserts the stop returns promptly and the commit still completes with the durable lock released. should pass.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass (targeted suites: 128 passed; full tests/run_agent/ + gateway/interrupt suites: 1952 passed, 7 pre-existing failures reproduced on clean main)
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15.4 (arm64), Python 3.11.15

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A (pure-Python threading change, no platform-specific code)
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

…mpression commit

During a compression commit the worker retains the commit-fence lock
until finish_commit(), and AIAgent.interrupt(hard_cancel=True) admitted
the stop via the BLOCKING cancel_before_commit — pinning the stop
caller (and the _pending_redirect_lock it holds) behind the whole
SessionDB mutation. On a large session (NousResearch#98351) that commit can run for
minutes, so /stop and /new stopped answering while the desktop showed a
permanent "Summarizing thread" overlay, Ctrl+C's signal handler hung,
and the gateway turn-timeout watchdog deadlocked interrupting the very
turn it was supposed to reap.

Admit the stop through the non-blocking try_cancel_before_commit()
instead: when a commit is already in flight the event is published
immediately. The commit is still never abandoned mid-mutation, the
conversation thread still observes the stop as soon as the compression
call returns, and the end state is identical — every stop producer only
sets the flag; none consumes "commit settled". The blocking form stays
as the fallback for fences without the non-blocking API.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Aug 30, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference; please use your judgment.

Overall: Makes /stop non-blocking during large-session compression commit.

What it does

  • run_agent.py:_admit_hard_cancel now tries CompressionCommitFence.try_cancel_before_commit first (non-blocking) and sets event immediately when commit in flight; falls back to legacy blocking cancel_before_commit(fence, event) if non-blocking form absent.
  • Rationale: worker retains fence lock until finish_commit(), so blocking path pinned stop caller and _pending_redirect_lock behind whole SessionDB mutation, hanging /stop and gateway watchdog on 1500+ message sessions (Desktop: 1500+ message session permanently stuck in 'summarizing thread' loop, UI completely dead #98351). Commit still runs to completion, never abandoned.
  • Test renamed to test_hard_stop_returns_promptly_while_commit_in_flight now asserts stop returns within 2s while commit blocked, and final state still identical.

Non-blocking notes

  • Broad except Exception around try_cancel logs debug and falls through to blocking path — safe, but if both paths fail silently the stop could be lost; debug log helps.

No functional issues found.

Non-blocking — please use your judgment.

@liuhao1024

Copy link
Copy Markdown
Contributor Author

Thanks for the review. On the except Exception note: the broad catch around try_cancel_before_commit is deliberate — it guards the hasattr fallback for older fence objects that only expose the blocking cancel_before_commit, so any failure there lands in the legacy blocking path (which is the pre-fix behavior and cannot be silently lost). The debug log is the right level for that expected-fallback case.

This branch has not been deployed

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

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Desktop: 1500+ message session permanently stuck in 'summarizing thread' loop, UI completely dead

3 participants