From 91d44719f455f2988f3ebdd18768c283805706da Mon Sep 17 00:00:00 2001 From: MkDev11 Date: Thu, 8 Jan 2026 02:05:32 -0800 Subject: [PATCH] fix(web): resolve chat message loading race conditions and infinite loops (#30695) Co-authored-by: crazywoola <100913391+crazywoola@users.noreply.github.com> --- web/app/components/app/log/list.spec.tsx | 228 ++++++++++++++++++++ web/app/components/app/log/list.tsx | 252 +++++++++-------------- 2 files changed, 322 insertions(+), 158 deletions(-) create mode 100644 web/app/components/app/log/list.spec.tsx diff --git a/web/app/components/app/log/list.spec.tsx b/web/app/components/app/log/list.spec.tsx new file mode 100644 index 0000000000..81901c6cad --- /dev/null +++ b/web/app/components/app/log/list.spec.tsx @@ -0,0 +1,228 @@ +/** + * Tests for race condition prevention logic in chat message loading. + * These tests verify the core algorithms used in fetchData and loadMoreMessages + * to prevent race conditions, infinite loops, and stale state issues. + * See GitHub issue #30259 for context. + */ + +// Test the race condition prevention logic in isolation +describe('Chat Message Loading Race Condition Prevention', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.useFakeTimers() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + describe('Request Deduplication', () => { + it('should deduplicate messages with same IDs when merging responses', async () => { + // Simulate the deduplication logic used in setAllChatItems + const existingItems = [ + { id: 'msg-1', isAnswer: false }, + { id: 'msg-2', isAnswer: true }, + ] + const newItems = [ + { id: 'msg-2', isAnswer: true }, // duplicate + { id: 'msg-3', isAnswer: false }, // new + ] + + const existingIds = new Set(existingItems.map(item => item.id)) + const uniqueNewItems = newItems.filter(item => !existingIds.has(item.id)) + const mergedItems = [...uniqueNewItems, ...existingItems] + + expect(uniqueNewItems).toHaveLength(1) + expect(uniqueNewItems[0].id).toBe('msg-3') + expect(mergedItems).toHaveLength(3) + }) + }) + + describe('Retry Counter Logic', () => { + const MAX_RETRY_COUNT = 3 + + it('should increment retry counter when no unique items found', () => { + const state = { retryCount: 0 } + const prevItemsLength = 5 + + // Simulate the retry logic from loadMoreMessages + const uniqueNewItemsLength = 0 + + if (uniqueNewItemsLength === 0) { + if (state.retryCount < MAX_RETRY_COUNT && prevItemsLength > 1) { + state.retryCount++ + } + else { + state.retryCount = 0 + } + } + + expect(state.retryCount).toBe(1) + }) + + it('should reset retry counter after MAX_RETRY_COUNT attempts', () => { + const state = { retryCount: MAX_RETRY_COUNT } + const prevItemsLength = 5 + const uniqueNewItemsLength = 0 + + if (uniqueNewItemsLength === 0) { + if (state.retryCount < MAX_RETRY_COUNT && prevItemsLength > 1) { + state.retryCount++ + } + else { + state.retryCount = 0 + } + } + + expect(state.retryCount).toBe(0) + }) + + it('should reset retry counter when unique items are found', () => { + const state = { retryCount: 2 } + + // Simulate finding unique items (length > 0) + const processRetry = (uniqueCount: number) => { + if (uniqueCount === 0) { + state.retryCount++ + } + else { + state.retryCount = 0 + } + } + + processRetry(3) // Found 3 unique items + + expect(state.retryCount).toBe(0) + }) + }) + + describe('Throttling Logic', () => { + const SCROLL_DEBOUNCE_MS = 200 + + it('should throttle requests within debounce window', () => { + const state = { lastLoadTime: 0 } + const results: boolean[] = [] + + const tryRequest = (now: number): boolean => { + if (now - state.lastLoadTime >= SCROLL_DEBOUNCE_MS) { + state.lastLoadTime = now + return true + } + return false + } + + // First request - should pass + results.push(tryRequest(1000)) + // Second request within debounce - should be blocked + results.push(tryRequest(1100)) + // Third request after debounce - should pass + results.push(tryRequest(1300)) + + expect(results).toEqual([true, false, true]) + }) + }) + + describe('AbortController Cancellation', () => { + it('should abort previous request when new request starts', () => { + const state: { controller: AbortController | null } = { controller: null } + const abortedSignals: boolean[] = [] + + // First request + const controller1 = new AbortController() + state.controller = controller1 + + // Second request - should abort first + if (state.controller) { + state.controller.abort() + abortedSignals.push(state.controller.signal.aborted) + } + const controller2 = new AbortController() + state.controller = controller2 + + expect(abortedSignals).toEqual([true]) + expect(controller1.signal.aborted).toBe(true) + expect(controller2.signal.aborted).toBe(false) + }) + }) + + describe('Stale Response Detection', () => { + it('should ignore responses from outdated requests', () => { + const state = { requestId: 0 } + const processedResponses: number[] = [] + + // Simulate concurrent requests - each gets its own captured ID + const request1Id = ++state.requestId + const request2Id = ++state.requestId + + // Request 2 completes first (current requestId is 2) + if (request2Id === state.requestId) { + processedResponses.push(request2Id) + } + + // Request 1 completes later (stale - requestId is still 2) + if (request1Id === state.requestId) { + processedResponses.push(request1Id) + } + + expect(processedResponses).toEqual([2]) + expect(processedResponses).not.toContain(1) + }) + }) + + describe('Pagination Anchor Management', () => { + it('should track oldest answer ID for pagination', () => { + let oldestAnswerIdRef: string | undefined + + const chatItems = [ + { id: 'question-1', isAnswer: false }, + { id: 'answer-1', isAnswer: true }, + { id: 'question-2', isAnswer: false }, + { id: 'answer-2', isAnswer: true }, + ] + + // Update pagination anchor with oldest answer ID + const answerItems = chatItems.filter(item => item.isAnswer) + const oldestAnswer = answerItems[answerItems.length - 1] + if (oldestAnswer?.id) { + oldestAnswerIdRef = oldestAnswer.id + } + + expect(oldestAnswerIdRef).toBe('answer-2') + }) + + it('should use pagination anchor in subsequent requests', () => { + const oldestAnswerIdRef = 'answer-123' + const params: { conversation_id: string, limit: number, first_id?: string } = { + conversation_id: 'conv-1', + limit: 10, + } + + if (oldestAnswerIdRef) { + params.first_id = oldestAnswerIdRef + } + + expect(params.first_id).toBe('answer-123') + }) + }) +}) + +describe('Functional State Update Pattern', () => { + it('should use functional update to avoid stale closures', () => { + // Simulate the functional update pattern used in setAllChatItems + let state = [{ id: '1' }, { id: '2' }] + + const newItems = [{ id: '3' }, { id: '2' }] // id '2' is duplicate + + // Functional update pattern + const updater = (prevItems: { id: string }[]) => { + const existingIds = new Set(prevItems.map(item => item.id)) + const uniqueNewItems = newItems.filter(item => !existingIds.has(item.id)) + return [...uniqueNewItems, ...prevItems] + } + + state = updater(state) + + expect(state).toHaveLength(3) + expect(state.map(i => i.id)).toEqual(['3', '1', '2']) + }) +}) diff --git a/web/app/components/app/log/list.tsx b/web/app/components/app/log/list.tsx index a17177bf7e..410953ccf7 100644 --- a/web/app/components/app/log/list.tsx +++ b/web/app/components/app/log/list.tsx @@ -209,7 +209,6 @@ type IDetailPanel = { function DetailPanel({ detail, onFeedback }: IDetailPanel) { const MIN_ITEMS_FOR_SCROLL_LOADING = 8 - const SCROLL_THRESHOLD_PX = 50 const SCROLL_DEBOUNCE_MS = 200 const { userProfile: { timezone } } = useAppContext() const { formatTime } = useTimestamp() @@ -228,69 +227,103 @@ function DetailPanel({ detail, onFeedback }: IDetailPanel) { const [hasMore, setHasMore] = useState(true) const [varValues, setVarValues] = useState>({}) const isLoadingRef = useRef(false) + const abortControllerRef = useRef(null) + const requestIdRef = useRef(0) + const lastLoadTimeRef = useRef(0) + const retryCountRef = useRef(0) + const oldestAnswerIdRef = useRef(undefined) + const MAX_RETRY_COUNT = 3 const [allChatItems, setAllChatItems] = useState([]) const [chatItemTree, setChatItemTree] = useState([]) const [threadChatItems, setThreadChatItems] = useState([]) const fetchData = useCallback(async () => { - if (isLoadingRef.current) + if (isLoadingRef.current || !hasMore) return + // Cancel any in-flight request + if (abortControllerRef.current) { + abortControllerRef.current.abort() + } + + const controller = new AbortController() + abortControllerRef.current = controller + const currentRequestId = ++requestIdRef.current + try { isLoadingRef.current = true - if (!hasMore) - return - const params: ChatMessagesRequest = { conversation_id: detail.id, limit: 10, } - // Use the oldest answer item ID for pagination - const answerItems = allChatItems.filter(item => item.isAnswer) - const oldestAnswerItem = answerItems[answerItems.length - 1] - if (oldestAnswerItem?.id) - params.first_id = oldestAnswerItem.id + // Use ref for pagination anchor to avoid stale closure issues + if (oldestAnswerIdRef.current) + params.first_id = oldestAnswerIdRef.current + const messageRes = await fetchChatMessages({ url: `/apps/${appDetail?.id}/chat-messages`, params, }) + + // Ignore stale responses + if (currentRequestId !== requestIdRef.current || controller.signal.aborted) + return if (messageRes.data.length > 0) { const varValues = messageRes.data.at(-1)!.inputs setVarValues(varValues) } setHasMore(messageRes.has_more) - const newAllChatItems = [ - ...getFormattedChatList(messageRes.data, detail.id, timezone!, t('dateTimeFormat', { ns: 'appLog' }) as string), - ...allChatItems, - ] - setAllChatItems(newAllChatItems) + const newItems = getFormattedChatList(messageRes.data, detail.id, timezone!, t('dateTimeFormat', { ns: 'appLog' }) as string) - let tree = buildChatItemTree(newAllChatItems) - if (messageRes.has_more === false && detail?.model_config?.configs?.introduction) { - tree = [{ - id: 'introduction', - isAnswer: true, - isOpeningStatement: true, - content: detail?.model_config?.configs?.introduction ?? 'hello', - feedbackDisabled: true, - children: tree, - }] - } - setChatItemTree(tree) - - const lastMessageId = newAllChatItems.length > 0 ? newAllChatItems[newAllChatItems.length - 1].id : undefined - setThreadChatItems(getThreadMessages(tree, lastMessageId)) + // Use functional update to avoid stale state issues + setAllChatItems((prevItems: IChatItem[]) => { + const existingIds = new Set(prevItems.map(item => item.id)) + const uniqueNewItems = newItems.filter(item => !existingIds.has(item.id)) + return [...uniqueNewItems, ...prevItems] + }) } - catch (err) { + catch (err: unknown) { + if (err instanceof Error && err.name === 'AbortError') + return console.error('fetchData execution failed:', err) } finally { isLoadingRef.current = false + if (abortControllerRef.current === controller) + abortControllerRef.current = null } - }, [allChatItems, detail.id, hasMore, timezone, t, appDetail, detail?.model_config?.configs?.introduction]) + }, [detail.id, hasMore, timezone, t, appDetail, detail?.model_config?.configs?.introduction]) + + // Derive chatItemTree, threadChatItems, and oldestAnswerIdRef from allChatItems + useEffect(() => { + if (allChatItems.length === 0) + return + + let tree = buildChatItemTree(allChatItems) + if (!hasMore && detail?.model_config?.configs?.introduction) { + tree = [{ + id: 'introduction', + isAnswer: true, + isOpeningStatement: true, + content: detail?.model_config?.configs?.introduction ?? 'hello', + feedbackDisabled: true, + children: tree, + }] + } + setChatItemTree(tree) + + const lastMessageId = allChatItems.length > 0 ? allChatItems[allChatItems.length - 1].id : undefined + setThreadChatItems(getThreadMessages(tree, lastMessageId)) + + // Update pagination anchor ref with the oldest answer ID + const answerItems = allChatItems.filter(item => item.isAnswer) + const oldestAnswer = answerItems[answerItems.length - 1] + if (oldestAnswer?.id) + oldestAnswerIdRef.current = oldestAnswer.id + }, [allChatItems, hasMore, detail?.model_config?.configs?.introduction]) const switchSibling = useCallback((siblingMessageId: string) => { const newThreadChatItems = getThreadMessages(chatItemTree, siblingMessageId) @@ -397,6 +430,12 @@ function DetailPanel({ detail, onFeedback }: IDetailPanel) { if (isLoading || !hasMore || !appDetail?.id || !detail.id) return + // Throttle using ref to persist across re-renders + const now = Date.now() + if (now - lastLoadTimeRef.current < SCROLL_DEBOUNCE_MS) + return + lastLoadTimeRef.current = now + setIsLoading(true) try { @@ -405,15 +444,9 @@ function DetailPanel({ detail, onFeedback }: IDetailPanel) { limit: 10, } - // Use the earliest response item as the first_id - const answerItems = allChatItems.filter(item => item.isAnswer) - const oldestAnswerItem = answerItems[answerItems.length - 1] - if (oldestAnswerItem?.id) { - params.first_id = oldestAnswerItem.id - } - else if (allChatItems.length > 0 && allChatItems[0]?.id) { - const firstId = allChatItems[0].id.replace('question-', '').replace('answer-', '') - params.first_id = firstId + // Use ref for pagination anchor to avoid stale closure issues + if (oldestAnswerIdRef.current) { + params.first_id = oldestAnswerIdRef.current } const messageRes = await fetchChatMessages({ @@ -423,6 +456,7 @@ function DetailPanel({ detail, onFeedback }: IDetailPanel) { if (!messageRes.data || messageRes.data.length === 0) { setHasMore(false) + retryCountRef.current = 0 return } @@ -440,91 +474,36 @@ function DetailPanel({ detail, onFeedback }: IDetailPanel) { t('dateTimeFormat', { ns: 'appLog' }) as string, ) - // Check for duplicate messages - const existingIds = new Set(allChatItems.map(item => item.id)) - const uniqueNewItems = newItems.filter(item => !existingIds.has(item.id)) + // Use functional update to get latest state and avoid stale closures + setAllChatItems((prevItems: IChatItem[]) => { + const existingIds = new Set(prevItems.map(item => item.id)) + const uniqueNewItems = newItems.filter(item => !existingIds.has(item.id)) - if (uniqueNewItems.length === 0) { - if (allChatItems.length > 1) { - const nextId = allChatItems[1].id.replace('question-', '').replace('answer-', '') - - const retryParams = { - ...params, - first_id: nextId, + // If no unique items and we haven't exceeded retry limit, signal retry needed + if (uniqueNewItems.length === 0) { + if (retryCountRef.current < MAX_RETRY_COUNT && prevItems.length > 1) { + retryCountRef.current++ + return prevItems } - - const retryRes = await fetchChatMessages({ - url: `/apps/${appDetail.id}/chat-messages`, - params: retryParams, - }) - - if (retryRes.data && retryRes.data.length > 0) { - const retryItems = getFormattedChatList( - retryRes.data, - detail.id, - timezone!, - t('dateTimeFormat', { ns: 'appLog' }) as string, - ) - - const retryUniqueItems = retryItems.filter(item => !existingIds.has(item.id)) - if (retryUniqueItems.length > 0) { - const newAllChatItems = [ - ...retryUniqueItems, - ...allChatItems, - ] - - setAllChatItems(newAllChatItems) - - let tree = buildChatItemTree(newAllChatItems) - if (retryRes.has_more === false && detail?.model_config?.configs?.introduction) { - tree = [{ - id: 'introduction', - isAnswer: true, - isOpeningStatement: true, - content: detail?.model_config?.configs?.introduction ?? 'hello', - feedbackDisabled: true, - children: tree, - }] - } - setChatItemTree(tree) - setHasMore(retryRes.has_more) - setThreadChatItems(getThreadMessages(tree, newAllChatItems.at(-1)?.id)) - return - } + else { + retryCountRef.current = 0 + return prevItems } } - } - const newAllChatItems = [ - ...uniqueNewItems, - ...allChatItems, - ] - - setAllChatItems(newAllChatItems) - - let tree = buildChatItemTree(newAllChatItems) - if (messageRes.has_more === false && detail?.model_config?.configs?.introduction) { - tree = [{ - id: 'introduction', - isAnswer: true, - isOpeningStatement: true, - content: detail?.model_config?.configs?.introduction ?? 'hello', - feedbackDisabled: true, - children: tree, - }] - } - setChatItemTree(tree) - - setThreadChatItems(getThreadMessages(tree, newAllChatItems.at(-1)?.id)) + retryCountRef.current = 0 + return [...uniqueNewItems, ...prevItems] + }) } catch (error) { console.error(error) setHasMore(false) + retryCountRef.current = 0 } finally { setIsLoading(false) } - }, [allChatItems, detail.id, hasMore, isLoading, timezone, t, appDetail]) + }, [detail.id, hasMore, isLoading, timezone, t, appDetail, detail?.model_config?.configs?.introduction]) useEffect(() => { const scrollableDiv = document.getElementById('scrollableDiv') @@ -556,24 +535,11 @@ function DetailPanel({ detail, onFeedback }: IDetailPanel) { if (!scrollContainer) return - let lastLoadTime = 0 - const throttleDelay = 200 - const handleScroll = () => { const currentScrollTop = scrollContainer!.scrollTop - const scrollHeight = scrollContainer!.scrollHeight - const clientHeight = scrollContainer!.clientHeight + const isNearTop = currentScrollTop < 30 - const distanceFromTop = currentScrollTop - const distanceFromBottom = scrollHeight - currentScrollTop - clientHeight - - const now = Date.now() - - const isNearTop = distanceFromTop < 30 - // eslint-disable-next-line sonarjs/no-unused-vars - const _distanceFromBottom = distanceFromBottom < 30 - if (isNearTop && hasMore && !isLoading && (now - lastLoadTime > throttleDelay)) { - lastLoadTime = now + if (isNearTop && hasMore && !isLoading) { loadMoreMessages() } } @@ -619,36 +585,6 @@ function DetailPanel({ detail, onFeedback }: IDetailPanel) { return () => cancelAnimationFrame(raf) }, []) - // Add scroll listener to ensure loading is triggered - useEffect(() => { - if (threadChatItems.length >= MIN_ITEMS_FOR_SCROLL_LOADING && hasMore) { - const scrollableDiv = document.getElementById('scrollableDiv') - - if (scrollableDiv) { - let loadingTimeout: NodeJS.Timeout | null = null - - const handleScroll = () => { - const { scrollTop } = scrollableDiv - - // Trigger loading when scrolling near the top - if (scrollTop < SCROLL_THRESHOLD_PX && !isLoadingRef.current) { - if (loadingTimeout) - clearTimeout(loadingTimeout) - - loadingTimeout = setTimeout(fetchData, SCROLL_DEBOUNCE_MS) // 200ms debounce - } - } - - scrollableDiv.addEventListener('scroll', handleScroll) - return () => { - scrollableDiv.removeEventListener('scroll', handleScroll) - if (loadingTimeout) - clearTimeout(loadingTimeout) - } - } - } - }, [threadChatItems.length, hasMore, fetchData]) - return (
{/* Panel Header */}