Skip to content

fix(sam3_image): prevent unwrap panic caused by LruCache eviction during mixed queries#254

Merged
jamjamjon merged 3 commits into
jamjamjon:mainfrom
darkSuperman:main
May 30, 2026
Merged

fix(sam3_image): prevent unwrap panic caused by LruCache eviction during mixed queries#254
jamjamjon merged 3 commits into
jamjamjon:mainfrom
darkSuperman:main

Conversation

@darkSuperman

Copy link
Copy Markdown
Contributor

🐛 The Bug
Calling self.text_cache.get(&p.text).unwrap() occasionally panics with Option::unwrap() on a None value.
This happens when:
The number of unique prompts in a single batch exceeds the default LruCache capacity (16).
Or, a previously cached prompt is pushed to the tail (least recently used), and a subsequent query mixes old (tail) prompts with new uncached ones.

🕵️ Root Cause
The original code used self.text_cache.contains(&p.text) which only checks existence but does not promote the item to the Most Recently Used (MRU) position.
When new prompts are put() into the cache, the capacity overflows, evicting the oldest items (even if they are about to be accessed in the current batch).
The subsequent unwrap() encounters a None value.

🛠️ The Fix
Promote MRU: Replaced .contains() with .get(). This inherently promotes accessed items to the front, granting them immunity from eviction during the same batch processing.
Dynamic Resize: Dynamically check seen.len(). If it exceeds current capacity, .resize() the cache to guarantee the entire batch fits safely.
Avoid Double Encoding: Extracted encode_texts into a variable encoded_feats to prevent running the heavy vision-language model twice for the same inputs.

@jamjamjon

Copy link
Copy Markdown
Owner

@darkSuperman thanks for the excellent fix! The analysis is spot-on and the changes are clean — merged.

Apologies for the very late response; this slipped through the cracks. Really appreciate your patience and contribution.

@jamjamjon jamjamjon merged commit 18175ef into jamjamjon:main May 30, 2026
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants