Skip to content

Commit 2c5f8b0

Browse files
authored
🤖 fix: prevent double-compact during force compaction (#957)
## Problem When force compaction triggers during streaming and includes a "Continue with the current task" message, a second compaction could trigger immediately after the first completed. ## Root Cause The force compaction guard was keyed by `activeStreamMessageId`, which changes when streams change. The guard was reset when `canInterrupt` became false (stream ends). This caused: 1. Force compaction triggers with `activeStreamMessageId='msg-1'` 2. Compaction stream starts with new `activeStreamMessageId='compact-msg'` 3. Compaction completes, stream ends, `canInterrupt` becomes false 4. Reset effect runs: `forceCompactionTriggeredRef = null` 5. Continue message stream starts: `activeStreamMessageId='continue-msg'` 6. Guard check: `null !== 'continue-msg'` → fails → **double compact!** ## Fix Use a simple boolean flag that only resets when `shouldForceCompact` becomes false (i.e., when compaction successfully reduced context usage). This directly encodes the intended semantics: "don't re-trigger until usage actually drops." ## Changes - Extract `useForceCompaction` hook for clean separation and testability - Use boolean guard instead of message ID tracking - Reset guard only when `shouldForceCompact` becomes false - Add comprehensive tests verifying: - Basic trigger conditions - Double-compact prevention (the main bug fix) - Guard reset after successful compaction - Rapid prop change handling ## Testing ``` bun test src/browser/hooks/useForceCompaction.test.ts 8 pass 0 fail ``` _Generated with `mux`_
1 parent 57608fa commit 2c5f8b0

File tree

3 files changed

+328
-29
lines changed

3 files changed

+328
-29
lines changed

src/browser/components/AIView.tsx

Lines changed: 10 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ import { executeCompaction } from "@/browser/utils/chatCommands";
4444
import { useProviderOptions } from "@/browser/hooks/useProviderOptions";
4545
import { useAutoCompactionSettings } from "../hooks/useAutoCompactionSettings";
4646
import { useSendMessageOptions } from "@/browser/hooks/useSendMessageOptions";
47+
import { useForceCompaction } from "@/browser/hooks/useForceCompaction";
4748
import { useAPI } from "@/browser/contexts/API";
4849

4950
interface AIViewProps {
@@ -139,9 +140,6 @@ const AIViewInner: React.FC<AIViewProps> = ({
139140
undefined
140141
);
141142

142-
// Track if we've already triggered force compaction for this stream
143-
const forceCompactionTriggeredRef = useRef<string | null>(null);
144-
145143
// Extract state from workspace state
146144
const { messages, canInterrupt, isCompacting, loading, currentModel } = workspaceState;
147145

@@ -158,41 +156,24 @@ const AIViewInner: React.FC<AIViewProps> = ({
158156
// Show warning when: shouldShowWarning flag is true AND not currently compacting
159157
const shouldShowCompactionWarning = !isCompacting && autoCompactionResult.shouldShowWarning;
160158

161-
// Force compaction when live usage shows we're about to hit context limit
162-
useEffect(() => {
163-
if (
164-
!autoCompactionResult.shouldForceCompact ||
165-
!canInterrupt ||
166-
isCompacting ||
167-
forceCompactionTriggeredRef.current === activeStreamMessageId
168-
) {
169-
return;
170-
}
171-
172-
forceCompactionTriggeredRef.current = activeStreamMessageId ?? null;
159+
// Handle force compaction callback - memoized to avoid effect re-runs
160+
const handleForceCompaction = useCallback(() => {
173161
if (!api) return;
174162
void executeCompaction({
175163
api,
176164
workspaceId,
177165
sendMessageOptions: pendingSendOptions,
178166
continueMessage: { text: "Continue with the current task" },
179167
});
180-
}, [
181-
autoCompactionResult.shouldForceCompact,
168+
}, [api, workspaceId, pendingSendOptions]);
169+
170+
// Force compaction when live usage shows we're about to hit context limit
171+
useForceCompaction({
172+
shouldForceCompact: autoCompactionResult.shouldForceCompact,
182173
canInterrupt,
183174
isCompacting,
184-
activeStreamMessageId,
185-
workspaceId,
186-
pendingSendOptions,
187-
api,
188-
]);
189-
190-
// Reset force compaction trigger when stream ends
191-
useEffect(() => {
192-
if (!canInterrupt) {
193-
forceCompactionTriggeredRef.current = null;
194-
}
195-
}, [canInterrupt]);
175+
onTrigger: handleForceCompaction,
176+
});
196177

197178
// Auto-retry state - minimal setter for keybinds and message sent handler
198179
// RetryBarrier manages its own state, but we need this for interrupt keybind
Lines changed: 261 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,261 @@
1+
/**
2+
* Tests for useForceCompaction hook
3+
*
4+
* Verifies the force compaction behavior when usage exceeds the context limit.
5+
* The key invariant: force compaction should only trigger ONCE until usage drops below threshold.
6+
*
7+
* Bug being tested (now fixed):
8+
* When compaction completes and continues with a follow-up message, the guard was reset
9+
* because canInterrupt changed, allowing a second compaction to trigger immediately.
10+
*/
11+
12+
import { describe, test, expect, mock, beforeEach, afterEach, type Mock } from "bun:test";
13+
import { renderHook, act, cleanup } from "@testing-library/react";
14+
import { GlobalWindow } from "happy-dom";
15+
import { useForceCompaction, type ForceCompactionParams } from "./useForceCompaction";
16+
17+
describe("useForceCompaction", () => {
18+
let onTrigger: Mock<() => void>;
19+
20+
beforeEach(() => {
21+
// Set up DOM environment for React
22+
globalThis.window = new GlobalWindow() as unknown as Window & typeof globalThis;
23+
globalThis.document = globalThis.window.document;
24+
25+
onTrigger = mock(() => undefined);
26+
});
27+
28+
afterEach(() => {
29+
cleanup();
30+
globalThis.window = undefined as unknown as Window & typeof globalThis;
31+
globalThis.document = undefined as unknown as Document;
32+
});
33+
34+
test("triggers compaction when shouldForceCompact and canInterrupt are true", () => {
35+
renderHook(() =>
36+
useForceCompaction({
37+
shouldForceCompact: true,
38+
canInterrupt: true,
39+
isCompacting: false,
40+
onTrigger,
41+
})
42+
);
43+
44+
expect(onTrigger).toHaveBeenCalledTimes(1);
45+
});
46+
47+
test("does not trigger when shouldForceCompact is false", () => {
48+
renderHook(() =>
49+
useForceCompaction({
50+
shouldForceCompact: false,
51+
canInterrupt: true,
52+
isCompacting: false,
53+
onTrigger,
54+
})
55+
);
56+
57+
expect(onTrigger).not.toHaveBeenCalled();
58+
});
59+
60+
test("does not trigger when canInterrupt is false", () => {
61+
renderHook(() =>
62+
useForceCompaction({
63+
shouldForceCompact: true,
64+
canInterrupt: false,
65+
isCompacting: false,
66+
onTrigger,
67+
})
68+
);
69+
70+
expect(onTrigger).not.toHaveBeenCalled();
71+
});
72+
73+
test("does not trigger when isCompacting is true", () => {
74+
renderHook(() =>
75+
useForceCompaction({
76+
shouldForceCompact: true,
77+
canInterrupt: true,
78+
isCompacting: true,
79+
onTrigger,
80+
})
81+
);
82+
83+
expect(onTrigger).not.toHaveBeenCalled();
84+
});
85+
86+
test("does NOT trigger double compaction when continue message starts after compaction", () => {
87+
// This is the key test for the bug fix!
88+
//
89+
// Timeline being tested:
90+
// 1. Stream running, usage high: shouldForceCompact=true → compaction triggers
91+
// 2. Compaction runs, completes: canInterrupt becomes false briefly
92+
// 3. Continue message starts: canInterrupt=true again, isCompacting=false
93+
// 4. BUG (before fix): second compaction would trigger because guard was reset
94+
// 5. FIX: guard stays set until shouldForceCompact becomes false
95+
96+
// Phase 1: Initial streaming state exceeding threshold
97+
const { rerender } = renderHook((props: ForceCompactionParams) => useForceCompaction(props), {
98+
initialProps: {
99+
shouldForceCompact: true,
100+
canInterrupt: true,
101+
isCompacting: false,
102+
onTrigger,
103+
},
104+
});
105+
106+
// First compaction should trigger
107+
expect(onTrigger).toHaveBeenCalledTimes(1);
108+
109+
// Phase 2: Compaction completes, stream ends briefly
110+
act(() => {
111+
rerender({
112+
shouldForceCompact: true, // Usage still high (compaction summary not yet reflected)
113+
canInterrupt: false, // Stream ended
114+
isCompacting: false,
115+
onTrigger,
116+
});
117+
});
118+
119+
// Still only 1 call (canInterrupt is false)
120+
expect(onTrigger).toHaveBeenCalledTimes(1);
121+
122+
// Phase 3: Continue message stream starts - THIS IS WHERE THE BUG WOULD OCCUR
123+
act(() => {
124+
rerender({
125+
shouldForceCompact: true, // Usage still high - would trigger second compaction without fix
126+
canInterrupt: true, // New stream started
127+
isCompacting: false,
128+
onTrigger,
129+
});
130+
});
131+
132+
// Key assertion: still only 1 call, NOT 2!
133+
expect(onTrigger).toHaveBeenCalledTimes(1);
134+
});
135+
136+
test("allows new compaction after usage drops below threshold", () => {
137+
// Phase 1: First compaction triggers
138+
const { rerender } = renderHook((props: ForceCompactionParams) => useForceCompaction(props), {
139+
initialProps: {
140+
shouldForceCompact: true,
141+
canInterrupt: true,
142+
isCompacting: false,
143+
onTrigger,
144+
},
145+
});
146+
147+
expect(onTrigger).toHaveBeenCalledTimes(1);
148+
149+
// Phase 2: Compaction succeeds, usage drops below threshold
150+
act(() => {
151+
rerender({
152+
shouldForceCompact: false, // Usage dropped!
153+
canInterrupt: false,
154+
isCompacting: false,
155+
onTrigger,
156+
});
157+
});
158+
159+
// Still only 1 call
160+
expect(onTrigger).toHaveBeenCalledTimes(1);
161+
162+
// Phase 3: Much later, usage builds up again and exceeds threshold
163+
act(() => {
164+
rerender({
165+
shouldForceCompact: true, // Usage exceeded again
166+
canInterrupt: true,
167+
isCompacting: false,
168+
onTrigger,
169+
});
170+
});
171+
172+
// New compaction should be allowed since usage dropped and rose again
173+
expect(onTrigger).toHaveBeenCalledTimes(2);
174+
});
175+
176+
test("does not re-trigger on prop changes while guard is active", () => {
177+
// Ensure multiple re-renders with same conditions don't trigger multiple times
178+
const { rerender } = renderHook((props: ForceCompactionParams) => useForceCompaction(props), {
179+
initialProps: {
180+
shouldForceCompact: true,
181+
canInterrupt: true,
182+
isCompacting: false,
183+
onTrigger,
184+
},
185+
});
186+
187+
expect(onTrigger).toHaveBeenCalledTimes(1);
188+
189+
// Re-render with same props (simulating React re-render)
190+
act(() => {
191+
rerender({
192+
shouldForceCompact: true,
193+
canInterrupt: true,
194+
isCompacting: false,
195+
onTrigger,
196+
});
197+
});
198+
199+
// Still only 1 call
200+
expect(onTrigger).toHaveBeenCalledTimes(1);
201+
202+
// Re-render again
203+
act(() => {
204+
rerender({
205+
shouldForceCompact: true,
206+
canInterrupt: true,
207+
isCompacting: false,
208+
onTrigger,
209+
});
210+
});
211+
212+
// Still only 1 call
213+
expect(onTrigger).toHaveBeenCalledTimes(1);
214+
});
215+
216+
test("maintains guard across rapid prop changes", () => {
217+
// Simulate rapid prop changes that might occur in React concurrent mode
218+
const { rerender } = renderHook((props: ForceCompactionParams) => useForceCompaction(props), {
219+
initialProps: {
220+
shouldForceCompact: true,
221+
canInterrupt: true,
222+
isCompacting: false,
223+
onTrigger,
224+
},
225+
});
226+
227+
// First trigger
228+
expect(onTrigger).toHaveBeenCalledTimes(1);
229+
230+
// Rapid transitions simulating complex UI updates
231+
act(() => {
232+
rerender({
233+
shouldForceCompact: true,
234+
canInterrupt: false, // Brief interruption
235+
isCompacting: false,
236+
onTrigger,
237+
});
238+
});
239+
240+
act(() => {
241+
rerender({
242+
shouldForceCompact: true,
243+
canInterrupt: true, // Back to interruptible
244+
isCompacting: true, // Now actually compacting
245+
onTrigger,
246+
});
247+
});
248+
249+
act(() => {
250+
rerender({
251+
shouldForceCompact: true,
252+
canInterrupt: true,
253+
isCompacting: false, // Compaction done
254+
onTrigger,
255+
});
256+
});
257+
258+
// Still only the initial trigger - guard prevented all re-triggers
259+
expect(onTrigger).toHaveBeenCalledTimes(1);
260+
});
261+
});
Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,57 @@
1+
/**
2+
* Force compaction hook - manages automatic compaction when context limit is approached
3+
*
4+
* Triggers compaction when:
5+
* - shouldForceCompact is true (usage exceeds threshold + buffer)
6+
* - canInterrupt is true (there's an active stream to interrupt)
7+
* - isCompacting is false (not already compacting)
8+
* - Haven't already triggered force compaction this cycle
9+
*
10+
* The key invariant: force compaction triggers ONCE per threshold breach.
11+
* The guard only resets when shouldForceCompact becomes false (successful compaction reduced context).
12+
*/
13+
14+
import { useEffect, useRef } from "react";
15+
16+
export interface ForceCompactionParams {
17+
shouldForceCompact: boolean;
18+
canInterrupt: boolean;
19+
isCompacting: boolean;
20+
onTrigger: () => void;
21+
}
22+
23+
/**
24+
* Hook to manage force compaction triggering
25+
*
26+
* @returns Whether force compaction has been triggered (for testing/debugging)
27+
*/
28+
export function useForceCompaction(params: ForceCompactionParams): boolean {
29+
const { shouldForceCompact, canInterrupt, isCompacting, onTrigger } = params;
30+
31+
// Track if we've already triggered force compaction (reset when usage drops)
32+
const forceCompactionTriggeredRef = useRef<boolean>(false);
33+
34+
// Force compaction when live usage shows we're about to hit context limit
35+
useEffect(() => {
36+
if (
37+
!shouldForceCompact ||
38+
!canInterrupt ||
39+
isCompacting ||
40+
forceCompactionTriggeredRef.current
41+
) {
42+
return;
43+
}
44+
45+
forceCompactionTriggeredRef.current = true;
46+
onTrigger();
47+
}, [shouldForceCompact, canInterrupt, isCompacting, onTrigger]);
48+
49+
// Reset force compaction trigger when usage drops below threshold (successful compaction)
50+
useEffect(() => {
51+
if (!shouldForceCompact) {
52+
forceCompactionTriggeredRef.current = false;
53+
}
54+
}, [shouldForceCompact]);
55+
56+
return forceCompactionTriggeredRef.current;
57+
}

0 commit comments

Comments
 (0)