fix: resolve_thread adopts existing session threads by UUID#377
fix: resolve_thread adopts existing session threads by UUID#377henrypark133 merged 2 commits intomainfrom
Conversation
When chat_new_thread_handler creates a thread directly in the session, it doesn't register a thread_map entry. On the first message, resolve_thread would create a duplicate thread with a different UUID, causing: - Thread appears empty when switching back (loadHistory queries the original UUID but turns live on the duplicate) - Orphaned tabs in the thread list (both the original and duplicate appear) Fix: before creating a new thread, check if the external_thread_id is itself a UUID that exists as a thread in the session. If so, adopt it and register the mapping. A mapped_elsewhere guard preserves channel scope isolation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
github-actions
Bot
added
scope: agent
Summary of ChangesHello @henrypark133, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses critical thread management issues within the session manager, specifically preventing the creation of duplicate threads and ensuring proper continuity when switching between existing threads. By enhancing the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request aims to fix a thread ID mismatch by adopting existing session threads by their UUID in resolve_thread. The logic seems correct and is accompanied by a new test case. The identified race condition is a critical issue that has been retained as it does not contradict any existing rules and provides valuable feedback.
| if !mapped_elsewhere { | ||
| let sess = session.lock().await; | ||
| if sess.threads.contains_key(&ext_uuid) { | ||
| drop(sess); | ||
| // Register mapping so future lookups are fast | ||
| let mut thread_map = self.thread_map.write().await; | ||
| thread_map.insert(key, ext_uuid); | ||
| drop(thread_map); | ||
| // Ensure undo manager exists | ||
| let mut undo_managers = self.undo_managers.write().await; | ||
| undo_managers | ||
| .entry(ext_uuid) | ||
| .or_insert_with(|| Arc::new(Mutex::new(UndoManager::new()))); | ||
| return (session, ext_uuid); | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a potential race condition in this block that could violate the intended channel-scoped thread isolation. The check for mapped_elsewhere is done under a read lock, which is then released. If another thread acquires a write lock and maps the same ext_uuid before this thread acquires its write lock for insertion, this thread will proceed to create a duplicate mapping. This would break the rule that a thread UUID should only be mapped from one channel.
To fix this, you should re-check if the ext_uuid is mapped after acquiring the write lock on thread_map but before inserting the new key. This double-checked locking approach ensures the check and insertion are effectively atomic with respect to other threads trying to map the same UUID.
if !mapped_elsewhere {
let sess = session.lock().await;
if sess.threads.contains_key(&ext_uuid) {
drop(sess);
let mut thread_map = self.thread_map.write().await;
// Re-check after acquiring write lock to prevent race condition.
if !thread_map.values().any(|&v| v == ext_uuid) {
// Register mapping so future lookups are fast
thread_map.insert(key, ext_uuid);
drop(thread_map);
// Ensure undo manager exists
let mut undo_managers = self.undo_managers.write().await;
undo_managers
.entry(ext_uuid)
.or_insert_with(|| Arc::new(Mutex::new(UndoManager::new())));
return (session, ext_uuid);
}
// If it was mapped elsewhere while we were unlocked, we fall through
// to create a new thread, preserving channel isolation.
}
}There was a problem hiding this comment.
Good catch — this is a real TOCTOU race. Applied double-checked locking: re-check thread_map.values().any(|&v| v == ext_uuid) after acquiring the write lock. If the UUID was mapped by another task in the interim, we fall through to create a new thread instead. All 8 resolve_thread tests pass. See ecc2378.
Re-check mapped_elsewhere after acquiring the write lock to prevent a TOCTOU race where another task could map the same UUID between the read lock check and write lock insertion, breaking channel isolation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
* fix: resolve_thread adopts existing session threads by UUID When chat_new_thread_handler creates a thread directly in the session, it doesn't register a thread_map entry. On the first message, resolve_thread would create a duplicate thread with a different UUID, causing: - Thread appears empty when switching back (loadHistory queries the original UUID but turns live on the duplicate) - Orphaned tabs in the thread list (both the original and duplicate appear) Fix: before creating a new thread, check if the external_thread_id is itself a UUID that exists as a thread in the session. If so, adopt it and register the mapping. A mapped_elsewhere guard preserves channel scope isolation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: double-checked locking in resolve_thread UUID adoption Re-check mapped_elsewhere after acquiring the write lock to prevent a TOCTOU race where another task could map the same UUID between the read lock check and write lock insertion, breaking channel isolation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
2 participants
Summary
chat_new_thread_handlercreates threads directly in the session without registering them inSessionManager.thread_mapresolve_threadcouldn't find the mapping → created a duplicate thread with a different UUIDloadHistoryreturned emptyFix
In
resolve_thread, before creating a new thread, check if theexternal_thread_idis itself a UUID that already exists as a thread in the session. If so, adopt it and register the mapping for future lookups. Amapped_elsewhereguard ensures channel-scoped isolation is preserved.Changes
src/agent/session_manager.rsresolve_thread+ new testTest plan
cargo fmt— cleancargo clippy --all --all-features— zero warningscargo test— 1,598 passed, 0 failedtest_resolve_thread_finds_existing_session_thread_by_uuidtest_register_then_resolve_different_channel_creates_new(channel isolation preserved)🤖 Generated with Claude Code