From f89501312a8749c22e5f769da64e69e691cc7fca Mon Sep 17 00:00:00 2001 From: zahica1234 Date: Fri, 29 May 2026 21:07:54 +0300 Subject: [PATCH] fix(chat): rearm safety timer on sub-agent tool / task-board updates (#2857) Co-authored-by: zahica1234 Co-authored-by: Claude Opus 4.7 --- app/src/pages/Conversations.tsx | 61 +++++++-- .../__tests__/Conversations.render.test.tsx | 123 +++++++++++++++++- 2 files changed, 174 insertions(+), 10 deletions(-) diff --git a/app/src/pages/Conversations.tsx b/app/src/pages/Conversations.tsx index 00a2609e6..3b93f3f8e 100644 --- a/app/src/pages/Conversations.tsx +++ b/app/src/pages/Conversations.tsx @@ -40,6 +40,7 @@ import { beginInferenceTurn, clearRuntimeForThread, fetchAndHydrateTurnState, + type InferenceStatus, setTaskBoardForThread, setToolTimelineForThread, } from '../store/chatRuntimeSlice'; @@ -296,6 +297,10 @@ const Conversations = ({ // from `selectedThreadId` so switching threads mid-turn doesn't move the // timer's reference point. const sendingThreadIdRef = useRef(null); + // Previous inference status for the sending thread; lets the rearm effect + // distinguish "status was just cleared (chat_done / chat_error)" from + // "status was never set yet (in-flight turn pre-status)". + const prevInferenceStatusRef = useRef(undefined); const getAudioExtension = (mimeType: string): string => { const lower = mimeType.toLowerCase(); @@ -493,32 +498,65 @@ const Conversations = ({ dispatch(setActiveThread(null)); sendingTimeoutRef.current = null; sendingThreadIdRef.current = null; + // Reset so the NEXT send starts from a clean "never had a status" + // baseline — otherwise the rearm effect could read this turn's last + // status as a stale "previous" and falsely treat the next send's + // first signal as a chat-done transition. + prevInferenceStatusRef.current = undefined; pendingSendRef.current = null; setPendingSendingThreadId(null); }, 120_000); }; // Rearm the silence timer on every inference signal for the sending - // thread. Tool / iteration / subagent events bump `inferenceStatusByThread`; - // pure-text streams (no tools) only bump `streamingAssistantByThread`, so - // both must be watched — otherwise a long text stream would trip the - // safety timer mid-reply. When the status is cleared (chat_done / - // chat_error), drop the timer — the completion handlers own UI cleanup. + // thread. Top-level tool / iteration events bump `inferenceStatusByThread`; + // pure-text streams (no tools) only bump `streamingAssistantByThread`; + // sub-agent activity (a delegated `Research`/`Tools Agent`/`Memory Tree` + // turn whose tools run in a child task) bumps `toolTimelineByThread` and + // `taskBoardByThread` without necessarily re-emitting a top-level status + // change, so all four must be watched — otherwise a long sub-agent loop + // would trip the safety timer mid-run even though the user can see the + // delegated tools firing in the timeline. When the status is cleared + // (chat_done / chat_error), drop the timer — the completion handlers + // own UI cleanup. + // + // `prevInferenceStatusRef` distinguishes "status was just cleared + // (chat_done / chat_error transition: defined → undefined)" from "status + // was never set yet (the Send handler also dispatches + // `setToolTimelineForThread({ entries: [] })` to reset the timeline, + // which fires this effect immediately after `armSilenceTimer` — at + // that instant the inference status hasn't been published yet)". Only + // the real transition should clear our timer. useEffect(() => { const threadId = sendingThreadIdRef.current; if (!threadId || !sendingTimeoutRef.current) return; const status = inferenceStatusByThread[threadId]; - if (status === undefined) { + if (status === undefined && prevInferenceStatusRef.current !== undefined) { clearTimeout(sendingTimeoutRef.current); sendingTimeoutRef.current = null; sendingThreadIdRef.current = null; + prevInferenceStatusRef.current = undefined; return; } + prevInferenceStatusRef.current = status; armSilenceTimer(threadId); - // armSilenceTimer is stable (refs + dispatch); depending on the - // selector references is enough to rearm on every progress event. + // Scope the dependencies to the SENDING thread's slices only, keyed by the + // reactive `activeThreadId` (set on send, cleared on done/error/timeout — + // so it tracks the in-flight turn for the timer's whole lifetime, unlike + // `pendingSendingThreadId` which is released the moment the backend accepts + // the send). Depending on the whole maps would rearm this thread's timer + // whenever ANY other thread's state changed — unrelated background activity + // shouldn't keep a foreground turn's timer alive. armSilenceTimer is stable + // (refs + dispatch), so listing the per-thread values is enough to rearm on + // every progress event for this thread. // eslint-disable-next-line react-hooks/exhaustive-deps - }, [inferenceStatusByThread, streamingAssistantByThread]); + }, [ + activeThreadId, + activeThreadId ? inferenceStatusByThread[activeThreadId] : undefined, + activeThreadId ? streamingAssistantByThread[activeThreadId] : undefined, + activeThreadId ? toolTimelineByThread[activeThreadId] : undefined, + activeThreadId ? taskBoardByThread[activeThreadId] : undefined, + ]); useEffect(() => { if ( @@ -745,6 +783,10 @@ const Conversations = ({ // changes for `sendingThreadId`, so long-running agent turns stay alive // as long as the backend is emitting signals. A truly hung server still // fails fast. + // Fresh send: clear the previous-status baseline before arming so the + // first inference signal of this turn isn't misread as a chat-done + // transition (defined → undefined) left over from the prior turn. + prevInferenceStatusRef.current = undefined; armSilenceTimer(sendingThreadId); dispatch(setToolTimelineForThread({ threadId: sendingThreadId, entries: [] })); dispatch(beginInferenceTurn({ threadId: sendingThreadId })); @@ -777,6 +819,7 @@ const Conversations = ({ sendingTimeoutRef.current = null; } sendingThreadIdRef.current = null; + prevInferenceStatusRef.current = undefined; const msg = err instanceof Error ? err.message : String(err); if ( msg.toLowerCase().includes('blocked by a security policy') || diff --git a/app/src/pages/__tests__/Conversations.render.test.tsx b/app/src/pages/__tests__/Conversations.render.test.tsx index 772e7631c..460aa6ed0 100644 --- a/app/src/pages/__tests__/Conversations.render.test.tsx +++ b/app/src/pages/__tests__/Conversations.render.test.tsx @@ -18,7 +18,10 @@ import { threadApi } from '../../services/api/threadApi'; import { chatSend } from '../../services/chatService'; import { CoreRpcError } from '../../services/coreRpcClient'; import agentProfileReducer from '../../store/agentProfileSlice'; -import chatRuntimeReducer from '../../store/chatRuntimeSlice'; +import chatRuntimeReducer, { + setInferenceStatusForThread, + setToolTimelineForThread, +} from '../../store/chatRuntimeSlice'; import socketReducer from '../../store/socketSlice'; import threadReducer, { setSelectedThread } from '../../store/threadSlice'; import type { Thread } from '../../types/thread'; @@ -802,6 +805,124 @@ describe('Conversations — smoke render (#1123 welcome-lock removal)', () => { } }); + it('rearms the silence timer on sub-agent tool-timeline updates', async () => { + // Regression: when a delegated sub-agent (`Research`, `Tools Agent`, + // …) is running, the parent thread's `inferenceStatusByThread` and + // `streamingAssistantByThread` references can stay put while + // `toolTimelineByThread` and `taskBoardByThread` tick. The rearm + // effect must watch all four — otherwise a long sub-agent loop + // trips the 120s safety timer even though the user can see tools + // firing in the timeline. + vi.useFakeTimers({ shouldAdvanceTime: true }); + try { + const { textarea, store, thread } = await renderSelectedConversation(); + + await act(async () => { + fireEvent.change(textarea, { target: { value: 'kick off a sub-agent loop' } }); + }); + await act(async () => { + fireEvent.click(screen.getByRole('button', { name: 'Send message' })); + }); + await waitFor(() => { + expect(chatSend).toHaveBeenCalledTimes(1); + }); + + // Two-thirds of the way through the safety window, the parent + // status is already in `subagent` phase and a delegated tool + // posts a timeline update. After the fix this re-arms the timer. + await act(async () => { + await vi.advanceTimersByTimeAsync(80_000); + }); + await act(async () => { + store!.dispatch( + setInferenceStatusForThread({ + threadId: thread.id, + status: { phase: 'subagent', iteration: 1, maxIterations: 8 }, + }) + ); + store!.dispatch( + setToolTimelineForThread({ + threadId: thread.id, + entries: [{ id: 'tl-1', name: 'web_fetch', round: 1, status: 'running' }], + }) + ); + }); + + // Advance another 80s (total elapsed 160s, well past the 120s + // window). The tool-timeline dispatch should have re-armed the + // timer at the 80s mark, so the silence timer is now at 80s of + // its fresh 120s budget and has NOT fired. The pending guard + // therefore still holds and Send stays disabled — proof the + // rearm effect ran on a toolTimelineByThread change. + await act(async () => { + await vi.advanceTimersByTimeAsync(80_000); + }); + await act(async () => { + fireEvent.change(textarea, { target: { value: 'still typing while sub-agent runs' } }); + }); + expect(screen.getByRole('button', { name: 'Send message' })).toBeDisabled(); + } finally { + vi.useRealTimers(); + } + }); + + it('does NOT rearm the silence timer on an unrelated thread’s updates', async () => { + // Regression for the per-thread dependency scoping: the rearm effect must + // react only to the SENDING thread's slices. A different thread churning + // (background triage, another conversation) must not keep the foreground + // turn's 120s timer alive — otherwise a truly hung send never fails fast. + vi.useFakeTimers({ shouldAdvanceTime: true }); + try { + const { textarea, store } = await renderSelectedConversation(); + + await act(async () => { + fireEvent.change(textarea, { target: { value: 'send on the foreground thread' } }); + }); + await act(async () => { + fireEvent.click(screen.getByRole('button', { name: 'Send message' })); + }); + await waitFor(() => { + expect(chatSend).toHaveBeenCalledTimes(1); + }); + + // Churn an UNRELATED thread the whole time the foreground send is open. + // None of these dispatches target the sending thread ('send-thread'), + // so they must not rearm its timer. + await act(async () => { + await vi.advanceTimersByTimeAsync(80_000); + }); + await act(async () => { + store!.dispatch( + setInferenceStatusForThread({ + threadId: 'some-other-thread', + status: { phase: 'subagent', iteration: 3, maxIterations: 8 }, + }) + ); + store!.dispatch( + setToolTimelineForThread({ + threadId: 'some-other-thread', + entries: [{ id: 'other-1', name: 'web_fetch', round: 1, status: 'running' }], + }) + ); + }); + + // Cross the original 120s deadline (80s + 50s = 130s). Because the + // unrelated-thread churn did NOT rearm, the safety timer fires: the + // pending guard is released and Send re-enables once the user types. + await act(async () => { + await vi.advanceTimersByTimeAsync(50_000); + }); + await act(async () => { + fireEvent.change(textarea, { target: { value: 'retry after timeout' } }); + }); + await waitFor(() => { + expect(screen.getByRole('button', { name: 'Send message' })).not.toBeDisabled(); + }); + } finally { + vi.useRealTimers(); + } + }); + it('releases the pending-send lock when chatSend rejects', async () => { vi.mocked(chatSend).mockRejectedValueOnce(new Error('emit failed')); const { textarea } = await renderSelectedConversation();