-
Notifications
You must be signed in to change notification settings - Fork 11
🤖 Fix race condition in auto-compact-continue #334
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
The issue: checkAutoCompact() can run concurrently (effect + store subscription), and both runs could pass the initial guard check before either sets the guard. Flow: - User runs /compact -c "continue..." - Backend completes compaction, sends events - Store updates → triggers subscription callback - Effect also triggers - Both call checkAutoCompact() nearly simultaneously Race condition: 1. Run #1: checks guard (not set) → proceeds 2. Run #2: checks guard (not set yet) → proceeds 3. Run #1: sets guard → sends message 4. Run #2: sets guard (redundant) → sends message (DUPLICATE!) Fix: Double-check guard immediately before setting it. Since JS is single-threaded, whichever run reaches the guard check first will set it, and the other will see it set and skip. Added logging to track when continue messages are sent for debugging.
e96d6aa to
4df4d7e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Why Both Checks Are NeededThis hook calls
Both are necessary. Removing either would break functionality. Why the Double-CheckThe race occurs because the store doesn't batch updates - each The double-check (line 70) ensures only one of the overlapping calls proceeds. It's defensive coding, but it's the pragmatic fix. Alternative: Batch Store UpdatesThe root cause is architectural - However, that's a bigger change affecting core store behavior. The double-check is simpler and sufficient for now. We can revisit store batching if we see similar issues elsewhere. |
## Problem After running `/compact -c "continue message"`, the continue message is sometimes sent **multiple times in a row** instead of just once. ## Root Cause The previous workspace-level guard had a race condition: 1. **Backend sends two events** for `replaceChatHistory`: delete event, then new message event 2. **Each event causes immediate synchronous `bump()`** which notifies all subscribers 3. **Multiple `checkAutoCompact()` calls can be in-flight simultaneously**: - Delete event → `bump()` → subscriber fires → `checkAutoCompact()` starts - New message event → `bump()` → subscriber fires → `checkAutoCompact()` starts - Both calls check workspace guard **before either sets it** → both proceed to send The double-check guard in PR #334 helped but didn't fully solve it because the checks and sets are separate operations. **The real issue: Workspace-level tracking is the wrong granularity.** We need to prevent processing the same compaction MESSAGE multiple times, not the same workspace multiple times. A new compaction creates a new message with a new ID. ## Solution Track processed **message IDs** instead of workspace IDs: ```typescript // Track which specific compaction summary messages we've already processed const processedMessageIds = useRef<Set<string>>(new Set()); // In checkAutoCompact: const messageId = summaryMessage.id; // Have we already processed this specific compaction message? if (processedMessageIds.current.has(messageId)) continue; // Mark THIS MESSAGE as processed processedMessageIds.current.add(messageId); ``` ## Why This Is Obviously Correct 1. **Message IDs are unique and immutable** - Once a message exists, its ID never changes 2. **We only care about processing each message once** - Not about processing each workspace once 3. **The guard is set BEFORE the async operation** - No timing window 4. **Multiple calls can overlap** - But they all see the same message ID, so only the first one proceeds 5. **Cleanup is natural** - IDs accumulate bounded (one per compaction) and don't need explicit cleanup The correctness is self-evident: "Have we sent a continue message for THIS compaction result message? Yes/No." ## How It Fixes The Race **Before (workspace-level):** - Call #1 checks `firedForWorkspace.has(workspaceId)` → false - Call #2 checks `firedForWorkspace.has(workspaceId)` → false (still!) - Call #1 sets guard and sends - Call #2 double-checks... but timing window existed **After (message-level):** - Call #1 checks `processedMessageIds.has(messageId)` → false - Call #2 checks `processedMessageIds.has(messageId)` → false (same message) - Call #1 adds messageId to set - Call #2 sees messageId already in set → skips Both calls are checking the **same unique identifier** (the message ID), so the guard works correctly even with concurrent execution. ## Testing Manual testing needed: 1. Run `/compact -c "continue message"` multiple times 2. Verify only ONE continue message is sent per compaction 3. Check console logs for single "Sending continue message" per compaction 4. Verify backend receives only one user message (not duplicates) _Generated with `cmux`_
Problem
Sometimes after compaction with a continue message (
/compact -c "message"), the continue message is sent multiple times in a row, suggesting a race condition.Root Cause
The
useAutoCompactContinuehook callscheckAutoCompact()from two places:When the store updates rapidly after compaction completes, both can execute concurrently:
Both runs pass the initial guard check because Run #2 checks before Run #1 sets the guard.
Solution
Double-check the guard immediately before setting it:
Since JavaScript is single-threaded, once Run #1 sets the guard, Run #2's double-check will see it and abort.
Changes
checkAutoCompact()Testing
The fix relies on JavaScript's single-threaded execution. The double-check ensures that even if two calls start concurrently, only one can proceed past the final guard check.
Manual testing should confirm no more duplicates, but the fix is sound by construction.
Generated with
cmux