Skip to content

Conversation

@hannesrudolph
Copy link
Collaborator

@hannesrudolph hannesrudolph commented Dec 16, 2025

Summary

Fixes a race condition bug where deleting a user message from chat history incorrectly removed assistant API messages that logically preceded the deleted message.

Problem

When deleting a user_feedback clineMessage, the system was using that message's timestamp as the cutoff for filtering API messages. Due to async execution during streaming (tool execution happens concurrently with stream completion), a clineMessage could have a timestamp BEFORE the corresponding assistant API message. This caused the assistant message to be incorrectly removed.

Timeline Example (Bug Scenario)

Timestamp Message Type Expected
T1=100 clineMessage "user_feedback" Delete
T2=200 API assistant (tool_use) Keep
T3=300 API user (tool_result) Delete

With the old logic (ts < 100), both API messages at T2 and T3 were removed.

Solution

Modified src/core/message-manager/index.ts to detect the race condition scenario by checking:

  1. If there's no exact timestamp match between clineMessage and API messages
  2. If there are messages before the cutoff that should be preserved

When both conditions are true (indicating a race condition), the fix finds the first API user message at or after the cutoff and uses that as the actual boundary. This ensures assistant messages that logically preceded the user's response are preserved.

Changes

  • src/core/message-manager/index.ts - Added race condition detection and handling in truncateApiHistoryWithCleanup
  • src/core/message-manager/index.spec.ts - Added 4 new test cases covering:
    • Race condition scenario (clineMessage timestamp earlier than assistant API message)
    • Normal case (timestamps properly aligned)
    • Fallback when no user message found at or after cutoff
    • Multiple assistant messages in race condition

Important

Fixes race condition in message deletion logic by adjusting cutoff timestamp handling in truncateApiHistoryWithCleanup to ensure correct message preservation.

  • Behavior:
    • Fixes race condition in truncateApiHistoryWithCleanup in index.ts by adjusting cutoff timestamp logic to preserve assistant messages when clineMessage timestamp is earlier.
    • Uses first API user message at or after cutoff as boundary if no exact match and messages exist before cutoff.
  • Tests:
    • Adds 4 test cases in index.spec.ts for race condition handling, normal timestamp alignment, fallback when no user message at or after cutoff, and multiple assistant messages in race condition.

This description was created by Ellipsis for f6096b1. You can customize this summary. It will automatically update as commits are pushed.

…essage deletion

When deleting a user message from chat history, assistant API messages
were incorrectly removed due to a timestamp race condition. During async
tool execution, a clineMessage (e.g., user_feedback) could be created
BEFORE the assistant API message was added to history.

The fix finds the first API user message at or after the cutoff timestamp
and uses that as the actual boundary, ensuring assistant messages that
logically preceded the user's response are preserved.

Added 4 test cases covering:
- Race condition scenario
- Normal timestamp ordering
- Fallback when no user message found
- Multiple assistant messages in race condition
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. bug Something isn't working labels Dec 16, 2025
@roomote
Copy link
Contributor

roomote bot commented Dec 16, 2025

Oroocle Clock   See task on Roo Cloud

Re-review complete. No new issues found in the latest changes; the existing follow-up item remains.

  • Tighten/gate the new actualCutoff heuristic in truncateApiHistoryWithCleanup so it does not change semantics for non-race rewinds (e.g. assistant-message rewinds where the next API user message is much later).
Previous reviews

Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues.

@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Dec 16, 2025
…lineMessage

Adds handling for a second race condition scenario where the API user
message (tool_result) timestamp is BEFORE the clineMessage timestamp.
This can happen when the tool_result is logged before the user_feedback
clineMessage due to timing.

The fix:
1. Detects the inverse race pattern by finding the last assistant
   message before the cutoff and checking if there are user messages
   between that assistant and the cutoff
2. When detected, uses the last assistant timestamp + 1 as the cutoff
   to ensure we keep the assistant response but remove the user message
   that belongs to the turn being deleted

Added test case demonstrating the inverse race condition scenario.
@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Dec 16, 2025
@cte cte merged commit 69307ba into main Dec 16, 2025
13 checks passed
@cte cte deleted the fix/message-deletion-race-condition branch December 16, 2025 05:23
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Dec 16, 2025
@github-project-automation github-project-automation bot moved this from Triage to Done in Roo Code Roadmap Dec 16, 2025
@cte cte mentioned this pull request Dec 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. lgtm This PR has been approved by a maintainer size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants