Skip to content

fix(clarify): unblock wait_for_response on thread-scoped interrupt (salvages #25506) - #84560

Open
ayushnangia wants to merge 2 commits into
NousResearch:mainfrom
ayushnangia:fix/clarify-interrupt-wait
Open

fix(clarify): unblock wait_for_response on thread-scoped interrupt (salvages #25506)#84560
ayushnangia wants to merge 2 commits into
NousResearch:mainfrom
ayushnangia:fix/clarify-interrupt-wait

Conversation

@ayushnangia

Copy link
Copy Markdown
Contributor

Summary

Salvage of #25506 (@liuhao1024, May 14 — conflicting, author inactive, sweeper verdict keep_open/salvageability=high): the clarify wait_for_response loop never observes the thread-scoped interrupt flag, so /stop or an interrupt-mode message cannot unblock an agent waiting on a clarify prompt. The agent thread wedges for the full clarify timeout (600s default) — or forever when timeout <= 0 (unlimited mode). RC1 of #83889; anchor of family E in the stall triage #84047.

Root cause

The deadlock is a cycle. AIAgent.interrupt() propagates the thread-scoped signal to tool workers (run_agent.py:2696-2703, per the sweeper's review of #25506), but wait_for_response (tools/clarify_gateway.py) polls only its resolution event and the inactivity heartbeat. The clear_session cleanup that cancels pending clarify entries runs at end-of-run (gateway/run.py) — which is unreachable while the thread is blocked in this wait. Flag set, never observed; cleanup ready, never reached.

Changes

Validation

  • scripts/run_tests.sh tests/tools/test_clarify_interrupt_wait.py tests/tools/test_clarify_tool.py tests/tools/test_clarify_gateway.py → 48 passed, 0 failed.
  • Red proof: with the fix stashed, both interrupt tests fail on main (pytest --timeout=25 guard — on unfixed code the unlimited-wait case blocks forever, which is the bug).
  • Six regressions: bounded-wait unblock, unlimited-wait unblock, resolve-wins race, timeout-without-interrupt preserved, no leaked entry rows after an interrupted wait, heartbeat untouched.
  • Pre-existing failures in test_clarify_active_session_bypass / photon poll / thread-followup reproduce identically on clean main — unrelated.

Scope notes

Salvages NousResearch#25506 (liuhao1024): the wait loop now checks
tools.interrupt.is_interrupted() once per slice, so /stop and
interrupt-mode messages unblock a pending clarify instead of wedging the
agent thread for the full timeout (600s default) or forever in unlimited
mode. The end-of-run clear_session cleanup cannot fire while this wait
blocks, so the interrupt flag was set but never observed (NousResearch#83889 RC1).

Also folds yflmq001's callsite gap from the NousResearch#84119 review: an
interrupted wait now reports '[interrupted by user]' instead of the
misleading '[user did not respond within Nm]'.

Sweeper verdict on NousResearch#25506: keep_open, salvageability=high.
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(clarify): unblock wait_for_response on thread-scoped interrupt (salvages #25506)

Good salvage — checking is_interrupted() per slice in tools/clarify_gateway.py:wait_for_response correctly unblocks the bounded and the unlimited wait, and the cleanup test covers the entry leak. Two things to verify:

  1. Stale interrupt flag after the abort: wait_for_response breaks when is_interrupted() is true, but the thread's entry in tools.interrupt._interrupted_threads is only cleared by an explicit set_interrupt(False, thread_id=...) from the agent loop — this code path never clears it. If the same worker thread is reused (gateway executor threads are reused across turns) before the flag is cleared, the next wait_for_response on that thread would abort instantly. The interrupt lifecycle is presumably "cleared by AIAgent after the turn ends," but with the wait now exiting mid-turn, confirm who clears it and consider clearing here when the wait exits due to interrupt.
  2. Interrupt-vs-resolve race is untested: the docstring claims "a response that raced the interrupt and resolved first still wins," but there is no test where resolve and interrupt are genuinely concurrent — the current tests exercise each in isolation. The outcome depends on loop iteration order (interrupt check at loop top, resolve checked after the slice wait). A deterministic concurrent test would pin the contract.
  3. Minor: gateway/run.py:_clarify_callback_sync distinguishes interrupt from timeout by checking is_interrupted() after wait_for_response returns None; a near-simultaneous timeout+interrupt reports "[interrupted by user]" — cosmetic, acceptable.

@ayushnangia
ayushnangia force-pushed the fix/clarify-interrupt-wait branch from aa6e89f to 2fc928f Compare August 17, 2026 19:35
wait_for_response's interrupt exit left the thread-scoped flag set;
gateway executor threads are reused across turns, so the NEXT clarify
wait on that thread would abort instantly (Enough1122 review on
NousResearch#84560). The early-exit path now clears the flag for its own thread;
the agent loop's clear remains the owner of turn-scoped semantics.
@ayushnangia
ayushnangia force-pushed the fix/clarify-interrupt-wait branch from 2fc928f to 597bfcb Compare August 23, 2026 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages 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.

3 participants