fix(gateway): unblock clarify wait on /stop or interrupt-mode messages - #84119
fix(gateway): unblock clarify wait on /stop or interrupt-mode messages#84119yflmq001 wants to merge 1 commit into
Conversation
`wait_for_response` (tools/clarify_gateway.py) blocked the agent thread on a threading.Event with only an inactivity heartbeat poll — no check of the per-thread interrupt flag. When the gateway's interrupt path (`_interrupt_and_clear_session` → `request_hard_interrupt`) fires, it sets the agent thread's interrupt bit via tools.interrupt, but the blocked wait never observes it and the run's `finally` cleanup can't run (the thread is stuck inside the tool), so the clarify stays blocked until the full 600s timeout even though the user hit /stop. The user's typed reply is also dead — see NousResearch#83889 (multiplexed Feishu groups; same mechanism on every platform with text-fallback clarify). Fix: poll `tools.interrupt.is_interrupted()` each 1s slice, exactly like the existing inactivity heartbeat. On interrupt the wait returns None and the entry is cleaned up through the same exit path as a timeout; the gateway clarify callback now surfaces it as "[interrupted by user]" instead of a misleading "did not respond within Nm". Regression test signals the per-thread interrupt flag from a second thread while a clarify is pending and asserts the wait unblocks well before the timeout (verified: without the fix the test blocks the full 10s and fails).
Duplicate of #25506, which already implements the same interrupt check in the clarify wait loop. |
|
You're right — this is a duplicate of #25506 (same interrupt check in the One small addition from this PR that #25506 doesn't have, in case it helps there: when the interrupt unblocks the wait, the gateway's clarify callback ( |
|
Reopening: the duplicate rationale (vs #25506) no longer holds.
Keeping it open so maintainers can compare both lines; happy to close again if #84560 is preferred. |
fix(gateway): unblock clarify wait on /stop or interrupt-mode messages No blocking issues found. A few minor observations:
|
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.
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.
Fixes #83889 (root cause 1:
wait_for_responsenever observes interrupts).Bug Description
When an agent in a gateway session calls
clarify, the agent thread blocks intools/clarify_gateway.wait_for_responseon athreading.Eventwith a 1s-slice poll loop whose only side-channel is the inactivity heartbeat. The gateway's interrupt path (_interrupt_and_clear_session→request_hard_interrupt→ agent_set_interrupt(True, thread_id)) sets the per-thread interrupt flag intools.interrupt, but the blocked wait never checks it. Because the agent thread is stuck inside the tool, the run'sfinallycleanup (clear_sessioninrun_sync) cannot execute either, so the clarify stays blocked until the full timeout (600s default) —/stopdoes nothing, and any user reply typed in the meantime is dead. Confirmed on multiplexed Feishu groups (#83889); the same mechanism applies to every platform with text-fallback clarify.Root Cause
wait_for_response(tools/clarify_gateway.py:135) polls only:entry.event.wait(timeout=slice_s)— set byresolve_gateway_clarify/clear_sessiontouch_activity_if_due— inactivity heartbeatNo interrupt flag check. The per-thread interrupt mechanism (
tools.interrupt.is_interrupted()) is exactly designed for this: the wait runs on the agent thread, and/stopmarks that same thread. This is the same patterntools/environments/base.pyalready uses for the heartbeat — the interrupt check is missing.Fix
tools/clarify_gateway.py—wait_for_responsepollstools.interrupt.is_interrupted()each 1s slice (same frequency as the heartbeat, try/except-guarded like the heartbeat import). On interrupt it breaks, returnsNone, and the existing post-loop cleanup removes the entry — identical exit path to a timeout.gateway/run.py—_clarify_callback_syncnow checks the interrupt flag afterwait_for_responsereturns and surfaces"[interrupted by user]"instead of the misleading"[user did not respond within 10m]"timeout message (the agent is in the interrupted state and winds the turn down).How to Verify
The new test registers a pending clarify, signals the per-thread interrupt flag from a second thread 50ms later, and asserts
wait_for_responsereturnsNonewell before the 10s timeout. Verified as a genuine regression: reverting the fix makes the test block the full timeout and fail (11.3s,AssertionError); with the fix it returns in ~0.05s.Test Plan
tests/tools/test_clarify_gateway.py— 24 passed (incl. new interrupt test)tests/gateway/test_clarify_active_session_bypass.py— 25 passed with the above (no regression)tests/gateway/test_clarify_progress_leak.py,test_telegram_clarify_buttons.py,test_clarify_thread_followup_not_swallowed.py— 9 passedRisk Assessment
Low. The interrupt check only shortens a wait that would otherwise run to its full timeout; the return path and entry cleanup are byte-for-byte the same as an existing timeout exit.
tools.interruptis a stdlib-only module with no new dependencies, and the import is guarded (falls back to a no-op predicate, same as the heartbeat import).