-
Notifications
You must be signed in to change notification settings - Fork 285
Fix duplicate_service.py empty-store crash and concurrency bugs #2035
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| [ | ||
| { | ||
| "ticket_id": "t-001", | ||
| "text": "I cannot connect to WiFi" | ||
| }, | ||
| { | ||
| "ticket_id": "t-002", | ||
| "text": "VPN authentication fails repeatedly" | ||
| }, | ||
| { | ||
| "ticket_id": "t-003", | ||
| "text": "VPN connection keeps dropping" | ||
| } | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| # Empty init file |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| import sys | ||
| from unittest.mock import MagicMock, patch | ||
| import torch | ||
| import pytest | ||
|
|
||
| # Mock sentence_transformers at module level | ||
| _mock_st = MagicMock() | ||
| _mock_model = MagicMock() | ||
|
|
||
| def _fake_encode(text, convert_to_tensor=False): | ||
| import hashlib, torch | ||
| if 'VPN' in text: | ||
| seed = 42 | ||
| else: | ||
| seed = sum(ord(c) for c in text) | ||
| torch.manual_seed(seed) | ||
| return torch.nn.functional.normalize(torch.randn(1, 384), dim=1).squeeze(0) | ||
|
|
||
| _mock_model.encode.side_effect = _fake_encode | ||
| _mock_st.SentenceTransformer.return_value = _mock_model | ||
| _mock_st.util.pytorch_cos_sim = lambda a, b: torch.nn.functional.cosine_similarity( | ||
| a.unsqueeze(0), b, dim=1 | ||
| ).unsqueeze(0) | ||
| sys.modules["sentence_transformers"] = _mock_st | ||
|
|
||
| @pytest.fixture | ||
| def dup_svc(): | ||
| from backend.services.duplicate_service import DuplicateService | ||
| svc = DuplicateService() | ||
| svc._loaded = True | ||
| svc._load_failed = False | ||
| svc.model = _mock_model | ||
| return svc | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,61 @@ | ||||||||||||||
| import pytest | ||||||||||||||
| import torch | ||||||||||||||
| from unittest.mock import MagicMock, patch | ||||||||||||||
|
|
||||||||||||||
| # ─── 1. Empty store returns None ──────────────────────────────────────────── | ||||||||||||||
| def test_check_duplicate_empty_store_returns_none(dup_svc): | ||||||||||||||
| result = dup_svc.check_duplicate("VPN is not working") | ||||||||||||||
| assert result is None, "Empty store must return None, not raise RuntimeError" | ||||||||||||||
|
|
||||||||||||||
| # ─── 2. Exact duplicate detected ──────────────────────────────────────────── | ||||||||||||||
| def test_exact_duplicate_detected(dup_svc, tmp_path): | ||||||||||||||
| dup_svc.storage_file = str(tmp_path / "cache.json") | ||||||||||||||
| dup_svc.add_ticket("t-001", "VPN is not working") | ||||||||||||||
| result = dup_svc.check_duplicate("VPN is not working") | ||||||||||||||
| assert result is not None | ||||||||||||||
| assert result["duplicate_ticket_id"] == "t-001" | ||||||||||||||
| assert result["similarity_score"] >= 0.99 | ||||||||||||||
|
|
||||||||||||||
| # ─── 3. Below-threshold text returns None ─────────────────────────────────── | ||||||||||||||
| def test_below_threshold_returns_none(dup_svc, tmp_path): | ||||||||||||||
| dup_svc.storage_file = str(tmp_path / "cache.json") | ||||||||||||||
| dup_svc.add_ticket("t-001", "VPN is not working") | ||||||||||||||
| result = dup_svc.check_duplicate("The printer has no paper") | ||||||||||||||
| assert result is None | ||||||||||||||
|
|
||||||||||||||
| # ─── 4. Picks highest similarity among multiple tickets ────────────────────── | ||||||||||||||
| def test_picks_best_match_among_multiple(dup_svc, tmp_path): | ||||||||||||||
| dup_svc.storage_file = str(tmp_path / "cache.json") | ||||||||||||||
| dup_svc.add_ticket("t-001", "I cannot connect to WiFi") | ||||||||||||||
| dup_svc.add_ticket("t-002", "VPN authentication fails repeatedly") | ||||||||||||||
| dup_svc.add_ticket("t-003", "VPN connection keeps dropping") | ||||||||||||||
| result = dup_svc.check_duplicate("VPN keeps disconnecting") | ||||||||||||||
| assert result is not None | ||||||||||||||
| assert result["duplicate_ticket_id"] in ("t-002", "t-003") | ||||||||||||||
|
|
||||||||||||||
| # ─── 5. Unavailable service returns None ──────────────────────────────────── | ||||||||||||||
| def test_unavailable_returns_none(dup_svc): | ||||||||||||||
| dup_svc._loaded = False | ||||||||||||||
| result = dup_svc.check_duplicate("test") | ||||||||||||||
| assert result is None | ||||||||||||||
|
|
||||||||||||||
| # ─── 6. Thread safety — concurrent add+check does not raise ───────────────── | ||||||||||||||
| def test_concurrent_add_check_no_exception(dup_svc, tmp_path): | ||||||||||||||
| import threading | ||||||||||||||
| dup_svc.storage_file = str(tmp_path / "cache.json") | ||||||||||||||
| errors = [] | ||||||||||||||
| def add(i): | ||||||||||||||
| try: | ||||||||||||||
| dup_svc.add_ticket(f"t-{i}", f"ticket text {i}") | ||||||||||||||
| except Exception as e: | ||||||||||||||
| errors.append(e) | ||||||||||||||
| def check(): | ||||||||||||||
| try: | ||||||||||||||
| dup_svc.check_duplicate("ticket text 5") | ||||||||||||||
| except Exception as e: | ||||||||||||||
| errors.append(e) | ||||||||||||||
| threads = [threading.Thread(target=add, args=(i,)) for i in range(20)] | ||||||||||||||
| threads += [threading.Thread(target=check) for _ in range(10)] | ||||||||||||||
| for t in threads: t.start() | ||||||||||||||
| for t in threads: t.join() | ||||||||||||||
|
Comment on lines
+59
to
+60
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Split one-line Line 59 and Line 60 currently use multiple statements on one line; this violates configured lint rules and can fail CI. 💡 Suggested fix- for t in threads: t.start()
- for t in threads: t.join()
+ for t in threads:
+ t.start()
+ for t in threads:
+ t.join()📝 Committable suggestion
Suggested change
🧰 Tools🪛 Ruff (0.15.15)[error] 59-59: Multiple statements on one line (colon) (E701) [error] 60-60: Multiple statements on one line (colon) (E701) 🤖 Prompt for AI AgentsSource: Linters/SAST tools |
||||||||||||||
| assert not errors, f"Thread safety violation: {errors}" | ||||||||||||||
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.
The encoder mock creates tie scores, so “best-match” behavior is not truly validated.
Line 12-15 maps every VPN-containing text to the same seed/embedding. That makes multiple VPN tickets indistinguishable and can hide regressions in ranking logic.
💡 Suggested fix
📝 Committable suggestion
🤖 Prompt for AI Agents