Feat/acronym search 04 detection and retrieval - #7
Open
borkarsaish65 wants to merge 24 commits into
Open
Conversation
Adds Alembic with a no-op baseline revision for the pre-existing unmanaged translations table, plus a migration that creates acronym_mapping (id PK, expansions as JSONB array, description, composite index on acronym+is_active) and seeds it from data/acronyms.csv, now the real 599-acronym dataset.
get_expansion() checks Redis first, falls back to Postgres on miss, writes through to cache; warm_cache() pre-populates every active acronym at startup. Redis pieces (cache_client.py, redis_client.py) stay acronym-agnostic; only acronym_service.py knows about acronyms, JSON-encoding the expansions array for the string-only cache layer.
get_expansion() only handled a cache miss (Redis says "not found"), not a cache error (Redis unreachable/timing out) — the latter raised straight out of the function before Postgres was ever tried, so a Redis outage broke every acronym-detected search instead of just degrading it. Now both the cache read and the write-through are guarded: any Redis error logs a warning and falls back to Postgres, matching the fallback behavior main.py's startup path already assumes exists.
POST /api/acronyms/bulk, gated behind a shared-secret X-Internal-Token
header (INTERNAL_API_TOKEN env var, no default). Upserts a batch of
{acronym, expansion, is_active} rows in a single transaction — a row
that's blank after trimming, or an in-batch duplicate acronym (Postgres
can't affect the same row twice in one ON CONFLICT statement), is
collected as a per-row error instead of failing the whole batch, with
last-value-wins for duplicates. Invalidates the Redis cache for every
acronym actually upserted so the next lookup re-fetches the new value.
Third slice of the acronym-search feature (see ACRONYM_SEARCH_PLAN.md).
# Conflicts:
# app/services/acronym_service.py
Was blank, which read as easy to miss when setting up a new environment. Placeholder, not a real token — each env still needs its own generated value per the comment above it.
Three issues found in review:
- invalidate_cache() (the bulk-upload endpoint's fallback when the
post-commit cache refresh fails) had no error handling of its own,
so the same Redis outage that triggered the fallback made the
fallback raise too — turning a fully successful, already-committed
upload into an opaque 500 for the caller. Now swallows and logs.
- Bulk upload decoded the file as strict utf-8, which leaves a
spreadsheet-exported BOM attached to the first header name
("acronym"), silently failing every row's acronym validation.
Switched to utf-8-sig, which strips a BOM if present and is
otherwise identical to utf-8.
- The shared Redis connection never forwarded REDIS_PASSWORD, so any
environment with a password-protected Redis would have every
acronym cache operation fail with AuthenticationError. Also
normalizes a blank password to None rather than "", since redis-py
treats an empty string as a real credential to AUTH with.
Adds tests/test_acronym_bulk_upload.py (BOM handling, cache-outage
500 regression) and commits tests/test_acronym_service.py, which was
still sitting uncommitted from PR2's review.
…loop Two more issues found in review: - bulk_upsert() only checked non-empty acronym/expansions before the single multi-row INSERT. An acronym longer than the acronym_mapping column's 32-char limit reached Postgres, raised StringDataRightTruncation, and rolled back the WHOLE batch — including every otherwise-valid row — contradicting the endpoint's own promise that one bad row doesn't fail the batch. Now validated per-row (length read off the model, not hardcoded) before the insert, alongside the existing non-empty check. - bulk_upload_acronyms is async def, but bulk_upsert() and warm_cache() are both fully synchronous, blocking I/O (sync SQLAlchemy Session, sync Redis client) with no actual awaits — warm_cache() alone does up to ~600 sequential blocking Redis writes. Called directly, this stalls the single-threaded event loop for its full duration, so every other in-flight request (search, health checks) queues up behind one acronym upload. warm_cache() converted from async def to a plain (honestly synchronous) function; both it and bulk_upsert() are now run via starlette.concurrency.run_in_threadpool from the endpoint and from app/main.py's startup lifespan. Verified live: 3 concurrent health checks each returned in ~20-30ms while a real upload (with its full cache refresh) was in flight, with no stacking delay. Tests added for both; two existing startup-lifespan tests updated from AsyncMock to Mock since warm_cache is no longer a coroutine.
detect_acronyms() (acronym_query_service.py) now actually drives search, not just logs: an acronym-detected query builds two separate dense embeddings (original + primary-expansion substituted, never concatenated) plus one combined BM25 OR-expansion string, fans them out one Qdrant request per field per embedding bundled into a single batched call, and merges same-field duplicate hits with max() since the same point can now be found via more than one embedding. Gated behind ACRONYM_SEARCH_ENABLED (default false). Non-acronym queries, and acronym-enabled queries with no match, take an unchanged code path: _parallel_batch_search/_hybrid_batch_search stay byte-identical to their pre-acronym-feature form (verified diff-equal to HEAD, whitespace aside); all new fan-out/merge logic lives in new _parallel_batch_search_multi/ _hybrid_batch_search_multi siblings, called only when acronym detection actually produces >1 embedding. Verified live: "DIET" surfaces documents that spell out "District Institute of Education and Training" but never use the acronym itself; a synthetic ZQTX acronym + test document confirmed the same behavior end-to-end with a clean before/after (0.19 undifferentiated -> 1.0 top match once the mapping existed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
get_expansion()'s Postgres query had no error handling, so a DB outage (or the acronym table not existing yet) propagated all the way to search()'s generic handler and turned every search request into a 500 — even for queries with no acronym in them. Search never depended on Postgres before this feature. Now caught and treated the same as "not an acronym": logged, returns None, search degrades to its pre-acronym-feature behavior instead of failing outright. Verified live: stopped vecsvc-postgres, cleared the Redis cache entry to force the DB code path, confirmed the new warning fires and the search request still returns 200 with real results instead of 500. Devin-flagged (see prioritized_search_service.py:263 finding). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
build_dense_queries() passed an expansion directly as re.sub's *replacement string*, which Python interprets specially (\1, \g<name>, \t, etc.). An expansion containing a literal backslash — e.g. a pasted Windows path — either raised re.error (bad escape) or silently corrupted the substituted text. Expansions come from an unvalidated admin CSV bulk-upload, so one bad row broke every search containing that acronym. Fixed by passing a function replacement instead of a string one — a callable's return value is substituted literally, with no escape processing. Verified: reverted the fix locally to confirm the new regression tests actually fail with the exact predicted re.error before restoring it. Also verified live — inserted a real acronym_mapping row with a backslash-containing expansion, confirmed the search request returns 200 with the expansion substituted correctly (not corrupted) instead of crashing. Test row removed after. Devin-flagged (see acronym_query_service.py:74 finding). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
get_expansion() only wrote to Redis on a positive hit (row found). A miss (word is not an acronym) was never cached, so it re-queried Postgres every single time. detect_acronyms() checks every all-caps token in a query, so an ordinary query like "ANNUAL REPORT FOR NCERT 2024" opened one fresh DB round-trip per non-acronym word, on every request, forever — zero caching benefit for the common case. Now a miss is cached too, under a new, shorter REDIS_NEGATIVE_CACHE_TTL (1h vs the existing 24h positive TTL). Checked staleness risk first: bulk upload already unconditionally refreshes every acronym's cache entry via warm_cache() after any create/update, so a newly-added acronym overwrites a stale negative entry immediately regardless of TTL — the shorter TTL is defense-in-depth, not load-bearing. json.dumps(None) -> "null" reads back correctly through the existing `if cached is not None: return json.loads(cached)` path, so no separate sentinel or read-path change was needed. Verified: reverted the fix locally, confirmed 3 of the 4 new regression tests fail exactly as predicted, restored it. Also verified live — ran "ANNUAL REPORT FOR NCERT 2024" against real Redis/Postgres, confirmed REPORT/ANNUAL got negative-cached (TTL ~3600, value "null") while the real acronym NCERT got a normal positive entry (TTL ~86400, real expansion); then stopped Postgres entirely and re-ran the exact same query, which still succeeded via cache alone with zero DB round-trips. Postgres restarted, test cache keys cleaned up after. Devin-flagged (see acronym_service.py:47 finding). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
detect_acronyms() only checked `expansion is not None`, so an empty expansions list ([]) was accepted as a valid detected acronym — the JSONB column defaults to '[]' and only bulk_upsert() (the admin CSV upload path) validates non-empty before insert, so a row written via any other path (raw SQL, a future writer) could still have expansions=[]. That flowed into build_dense_queries' expansions[0] with no length check, raising IndexError -> search()'s generic handler -> 500. build_sparse_query didn't crash but produced a malformed trailing "... OR " clause. Fixed at the source: `if expansion:` (falsy check) instead of `if expansion is not None:`, so an empty list is rejected the same way as "acronym not found" — both downstream call sites are safe by construction since neither ever receives an empty-list acronym in its mapping. Verified: reverted the fix locally, confirmed the two new regression tests fail exactly as predicted, restored it. Also verified live — inserted a real acronym_mapping row with expansions=[] via raw SQL (bypassing bulk_upsert's own validation, exactly as the finding describes), confirmed the search request returns 200 with only the plain query embedded (no acronym substitution attempted) instead of crashing. Test row removed after. Devin-flagged (see acronym_query_service.py:50 finding). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
build_sparse_query()'s docstring claimed "structured boolean syntax
works correctly for BM25", and the code wrapped expansions in OR/
quotes accordingly (e.g. PTM OR "Parent Teacher Meeting"). False: the
sparse encoder (fastembed's Qdrant/bm25) is a plain bag-of-words
tokenizer with no notion of OR or quoted phrases.
Verified empirically before changing anything: encoding the wrapped
form and the equivalent plain word-concatenation produce byte-
identical token sets ("or" is stripped as a stopword either way, not
injected as noise). So the wrapping was inert decoration, not a
correctness bug — nothing scored differently because of it. Fixed as
a clarity cleanup: append expansion words directly instead of writing
text that looks structured but isn't, and corrected the docstring to
describe what actually happens.
Behavior-preserving, not a functional change — confirmed via a new
regression test asserting the old wrapped format and new plain format
tokenize identically, and verified live (DIET search results
unchanged in ranking pattern before/after).
Devin-flagged (see acronym_query_service.py:110 finding). Downgraded
from "impact: noisier ranking" in the original report — the specific
claims (literal "or" injected, wrapping causes different scoring than
plain concatenation) did not hold up under direct testing; the real
issue was a misleading docstring plus decorative-but-inert code, not
a scoring defect.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
build_dense_queries() compared the substituted query against the
original case-sensitively. query_for_embedding is always lowercased
upstream (preprocess_query), but expansions keep their stored casing
— so for ~49 real seeded acronyms whose expansion is just a re-cased
version of the acronym itself (BLUETOOTH -> "Bluetooth", DIGILOCKER
-> "DigiLocker", VEDANTU -> "Vedantu", ...), substitution produced a
"different" string that was really the same query. That triggered
search()'s use_acronym_multi_query fan-out path, doubling the
per-field Qdrant requests for an embedding that's numerically
identical to the first.
Confirmed empirically before fixing: cosine similarity between
embed_query("bluetooth") and embed_query("Bluetooth") is exactly 1.0
— the second round-trip contributed nothing.
Fixed with a case-insensitive comparison. Verified: reverted locally,
confirmed the two new regression tests fail exactly as predicted,
restored it. Also verified live — searching "BLUETOOTH" now generates
1 dense embedding and fans out across 6 fields (was 2 embeddings / 11
fields); confirmed "DIET" (a genuine expansion) still correctly
generates 2, so the fix doesn't over-suppress real cases.
Devin-flagged (see acronym_query_service.py:91 finding).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ght) Two Devin-flagged findings addressed via data curation rather than code changes: acronym_query_service.py:43 — 9 seeded rows were common English words (THE, SET, ACT, PROJECT, INTERNAL, ORAL, LIBRARY, HOSTEL, CANTEEN). detect_acronyms() checks any all-caps token regardless of dictionary- word status, so an all-caps query like "THE NEW EDUCATION POLICY" silently detected THE and produced a second search variant for "Times Higher Education" — a completely unrelated topic, competing on equal footing via the max()-merge ranking. Removed from the seed CSV and deactivated in the live DB (is_active=false, not deleted, so the audit trail survives). acronym_query_service.py:12 — 161 of 599 rows contain spaces, so they can never be detected (detection is strictly per-token). Split into: 4 rows (BA LLB, BBA LLB, BCOM LLB, BSC LLB) were pure dead weight — both halves already exist as safe standalone entries and are already independently detectable, so the combined row added nothing. Removed. 3 new safe, specific, reusable standalone entries split out from multi-word combinations: JRF (from CSIR/DBT/ICMR/UGC JRF), UG (from UG DIPLOMA/NEET UG), BSE (from BSE ODISHA/TELANGANA). The remaining ~147 multi-word entries were deliberately NOT split — most of their constituent words (CLUB, QUOTA, BOARD, CLASS, HEALTH, CODE, BUS, TEST, ...) are generic enough to reintroduce the exact false-positive class just fixed above, or are Hindi/proper-noun fragments from scheme names that aren't real standalone acronyms (BHARAT, SHIKSHA, POSHAN, ...). True multi-token/phrase detection for those remains the already- deferred gap from 2026-08-14 (see ACRONYM_SEARCH_PROGRESS.md), out of scope here. Net: 599 -> 589 rows (-13, +3). Also fixes tests/test_acronym_mapping.py's two hardcoded `count == 599` assertions (now 589) — caught by re-running the full suite after this change, confirming the migration/seed tests are honest about the new count rather than silently stale. New tests/test_acronym_seed_data.py: pure CSV-content regression tests guarding against the 9 dangerous words or 4 redundant entries being silently reintroduced by a future edit, and confirming JRF/UG/BSE are present with the correct expansions. Verified live: confirmed THE NEW EDUCATION POLICY no longer detects THE or generates the bogus second embedding (also had to manually flush a stale Redis cache entry for THE left over from earlier in-session testing — direct SQL writes bypass the app's normal cache-invalidation path). Confirmed JRF/UG/BSE now detect correctly, and BA LLB's removal is safe (BA and LLB still independently detect). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…on/digits Second half of the acronym_query_service.py:12 finding (161 unreachable entries) — the first pass handled the 151 multi-word ones; this handles the remaining 10 single-token entries containing a hyphen, apostrophe, or digit (WI-FI, E-PATHSHALA, DDU-GKY, ANTI-BULLYING, CO-SCHOLASTIC, PRE-BOARD, RE-EVALUATION, BYJU'S, 4G, 5G). _normalize_token strips all non-letters before lookup, so none of these could ever match no matter what the user typed. Considered renaming them to their normalized form instead (e.g. WI-FI -> WIFI) but decided against it — not valuable enough to justify even that small a change. Removed from the seed CSV and deactivated in the live DB instead, same treatment as the earlier false-positive/redundant entries. Two things worth noting: - DDU-GKY normalizes to DDUGKY, which already existed as a separate, identical-expansion row — removed the duplicate rather than renaming into a collision. - 4G/5G are a harder problem than the rest, not just a bad rename candidate: _normalize_token strips digits entirely, so both collapse to the single letter "G" — below the length-2 detection guard AND colliding with each other. No rename could ever fix them without changing the normalization/guard logic itself, which is out of scope here (same "not now" call as multi-token detection). Net: 589 -> 579 rows (-10). Verified live: confirmed WI-FI/WIFI/4G/BYJU'S no longer detect, and that DDU-GKY (typed with the hyphen) still correctly resolves via the surviving DDUGKY entry. Manually flushed Redis cache entries for all 10 before verifying, learned from the earlier stale-cache miss on THE. Fixed tests/test_acronym_mapping.py's two row-count assertions again (589 -> 579). Extended test_acronym_seed_data.py with regression tests for this removal. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
User caught a pattern I should have scanned for exhaustively the first
time around: any row where the acronym itself appears (case-differing
or as a literal word) inside its own expansion isn't a real acronym —
either it's just a re-typed word/brand name, or it's an ordinary
English word with the same false-positive risk as THE/SET/ACT from
the first curation pass, just missed because Devin's report only gave
9 examples, not an exhaustive scan.
Filtered to single-word acronym keys only — multi-word keys in this
same pattern can't trigger detection at all (per-token limitation,
already handled separately), so they're irrelevant to this risk.
Removed (31 total):
- 13 not real acronyms at all, expansion is just the same word/brand
re-capitalized (BLUETOOTH -> Bluetooth, CLERK -> Clerk, DIGILOCKER
-> DigiLocker, ...). No longer cause a functional bug (the case-only
dense-query fix already stops the wasted double search) but aren't
legitimate acronym entries.
- 18 ordinary English words whose expansion just tacks on one more
word (VIVA -> Viva Voce, PRACTICAL -> Practical Examination,
BIOMETRIC -> Biometric Attendance, ...) — same false-positive class
as acronym_query_service.py:43's finding: an all-caps query
containing one of these would be silently rewritten into a
different search.
Kept (8): DUOLINGO, AMITY, MANIPAL, SRM, WEBOMETRICS, CHILDLINE,
PATRACHAR, INCINERATOR — real proper nouns/institution names, not
ordinary words at meaningful false-trigger risk.
Net: 579 -> 548 rows (-31).
Verified live: confirmed a realistic all-caps query containing VIVA
("SUBMIT YOUR VIVA REPORT BY FRIDAY") no longer gets silently
rewritten into a second search variant; confirmed AMITY (kept) still
detects correctly; confirmed BLUETOOTH (removed) no longer detects.
Flushed Redis cache entries for all 31 before verifying.
Fixed tests/test_acronym_mapping.py's two row-count assertions again
(579 -> 548). Extended test_acronym_seed_data.py with regression tests
for both removed groups and the kept group.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
redis.Redis(...) was constructed with no socket_timeout/socket_connect_ timeout (redis-py defaults both to None). A blackholed connection — network silently drops packets, unlike a clean "connection refused" — would hang every cache read/write until the OS-level TCP timeout, which can be minutes. This runs synchronously on the search request path, so an unresponsive cache would stall search entirely instead of degrading to Postgres per acronym_service.get_expansion()'s existing except/fallback. Confirmed live with a real local "black hole" TCP server (accepts the connection, never responds): the unfixed client hung past 20s with no sign of ever returning. Fixing this took two rounds, not one — the first pass (setting socket_timeout=5/socket_connect_timeout=3) looked right in isolation but still hung well past 20s live. Root cause: redis-py's own DEFAULT retry policy is 10 attempts with exponential backoff on ConnectionError/TimeoutError, so the configured timeout was being paid up to 10 times over before finally raising. Added retry=Retry(NoBackoff(), 0) to fail on the first attempt instead — get_expansion() already has its own fallback-to-Postgres logic, so retrying inside the Redis client itself is redundant and defeats "fail fast." (retry_on_timeout/retry_on_error are deprecated in this redis-py version; the zero-retry Retry object alone covers both error types.) Also tightened the timeout values themselves after live-verifying the first pass: get_expansion() makes TWO Redis calls on the fallback path (a read, then a write-through of the Postgres result), each independently paying the timeout on an outage, and detect_acronyms() calls get_expansion() once per candidate token — so the per-request cost multiplies. With 3s/5s, a single cache-miss during an outage took just over 10s before falling back. Healthy Redis responds in single-digit milliseconds, so tightened to 1s/1s — confirmed live this still resolves correctly, just faster (2.02s instead of 10.03s for the same scenario), and confirmed a real healthy lookup against the actual local Redis completes in ~7ms, nowhere near the new timeout. Devin-flagged (see redis_client.py:9 finding). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
verify_internal_token() compared the submitted token with plain `!=`, which short-circuits at the first differing character on Python str — response timing leaks how many leading characters an attacker guessed correctly, in principle letting them recover the shared secret byte-by-byte over the network. This guards POST /api/acronyms/bulk, a write path over the entire acronym dictionary, so the secret is worth protecting properly. Fixed with secrets.compare_digest, which runs in constant time for equal-length inputs. Guarded against None (Header(None) default) since compare_digest requires str/bytes on both sides. Verified: reverted the fix locally, confirmed the new test_uses_constant_time_comparison_not_plain_equality test fails exactly as predicted, restored it. Also verified live against the real running server — wrong token now returns 401, correct token 200, search endpoint unaffected. Devin-flagged (see deps.py:17 finding). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Found one round after the previous timing-safe-comparison fix: secrets.compare_digest raises TypeError on any non-ASCII str input. Starlette decodes headers as latin-1, so any header byte > 0x7F produces exactly that. The TypeError isn't an HTTPException, so it surfaced as an unhandled 500 instead of the intended 401 — confirmed live against the real running endpoint before this fix (X-Internal-Token: café-token -> 500 Internal Server Error). Worse, this makes the crash client-triggerable at will, and if INTERNAL_API_TOKEN itself were ever configured with a non-ASCII character, every single request to the endpoint would break. Fixed by encoding both sides to bytes (utf-8, surrogateescape) before comparing — encoding never raises, so the comparison is now total over every possible header value while staying constant-time via compare_digest on bytes. Verified live: the same café-token request now returns a clean 401, correct token still returns 200. New tests cover a non-ASCII header being rejected cleanly, a non-ASCII configured secret not crashing every request, and a genuinely matching non-ASCII token still authenticating correctly (not just "doesn't crash"). Devin-flagged (see deps.py:24 finding, second round on this function). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…other
build_dense_queries() looped over detected acronyms, calling pattern.sub
sequentially and feeding each result into the next iteration — so text
inserted by an earlier substitution was visible to (and re-matchable
by) later ones. The seeded dictionary has real cases where one
acronym's expansion contains another acronym as a plain word: NFST ->
"National Fellowship for ST", where ST is itself a real acronym
("Scheduled Tribes"). Query "NFST ST fellowship" detects both; the
NFST pass inserted "...for ST...", and the subsequent ST pass then
matched BOTH the freshly-inserted ST and the user's own ST token,
producing "National Fellowship for Scheduled Tribes Scheduled Tribes
fellowship" — garbled, duplicated text sent to the embedding model.
Confirmed empirically before fixing.
Fixed by resolving all detected acronyms in a single pass: one
alternation regex (\b(?:A|B|C)\b) matched against the ORIGINAL query
text once, with a callback resolving each match to its own expansion.
Since re.sub's single pass never re-scans its own output, inserted
expansion text can no longer be re-matched.
Verified: reverted to the old looped version locally, confirmed the
new regression test fails with the exact predicted garbled output,
restored the fix. All 31 pre-existing tests in this file still pass
unchanged — same substitution results for every case that isn't this
specific interaction. Verified live through the real search endpoint:
"NFST ST fellowship" now generates a second dense embedding of
"National Fellowship for ST Scheduled Tribes fellowship" instead of
the garbled duplicate.
Devin-flagged (see acronym_query_service.py:81 finding).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two-part fix, same pattern as the earlier Redis client fix:
1. psycopg2 had no connect_timeout (SQLAlchemy's create_engine default).
A blackholed Postgres host (network silently drops packets, unlike
a clean "connection refused") makes a connection attempt hang until
the OS-level TCP timeout, which can be minutes. Confirmed live with
a real local "black hole" TCP server: an unbounded psycopg2.connect()
hung past 15s with no sign of returning. Added connect_args=
{"connect_timeout": POSTGRES_CONNECT_TIMEOUT} (default 3s) to the
shared engine — every caller of SessionLocal()/get_db() benefits,
not just acronym lookups.
2. Even bounded, get_expansion()'s DB-error branch returned None
without caching anything (unlike the genuine-miss branch just below
it, which already writes a negative cache entry). detect_acronyms()
calls get_expansion() once per candidate token, so every token of
every search would re-attempt the (now-bounded, but still nonzero-
cost) Postgres connection for the entire duration of an outage —
exactly the repeated-round-trip cost the negative cache was added
to avoid, just left open on this one path. Added the same caching
here, under a new REDIS_DB_ERROR_CACHE_TTL (30s, much shorter than
the 1-hour REDIS_NEGATIVE_CACHE_TTL used for genuine misses) — an
outage is often transient, so this shouldn't keep real acronyms
unresolvable for long after Postgres recovers.
Verified: reverted the caching fix locally, confirmed the two new
regression tests fail exactly as predicted, restored it. Also verified
live end-to-end through the real get_expansion() call against a real
blackholed Postgres: first lookup bounded to 3.01s (was: hangs
indefinitely), second lookup for the same word 0.0006s (served
entirely from the negative cache, zero DB round-trip). Confirmed
healthy Postgres access is unaffected (24ms, same as before).
Devin-flagged (see acronym_service.py:54 finding).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ss noise Root cause: min-max normalization stretches whatever the candidate pool's best dense score is to 1.0 regardless of absolute magnitude, and the dense/sparse fusion always trusts dense at a fixed 70% weight — so a document with zero real word overlap can outrank one that genuinely contains the query's words, as long as nothing better exists in the pool (verified live: querying "SST" surfaced two zero-keyword-match documents above three documents that actually said "Social Science"). Two changes, both scoped to acronym-detected queries only (via a new is_acronym_query flag threaded through _process_and_filter_results -> _rank_results, defaulting to False so every other caller and every non-acronym query is provably unchanged): - _rank_results: per-point dense/sparse blend flips to favor sparse when that point has a genuine BM25 hit, instead of always trusting dense 70%. - search(): title/summary boost now also checks each detected acronym's expansion text (not just the literal acronym) against title/summary, so a document titled with the spelled-out expansion gets the same deliberate boost a literal match would. Verified live against the real corpus (SST, ZQXA queries) and the full test suite (131 passed, 1 pre-existing unrelated failure in Redis cache mock handling, confirmed to fail identically without this change). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previous commit (38109b9) always flipped a point's dense/sparse blend to 30/70 whenever it had any genuine sparse hit, regardless of how strong its own dense score already was. That could backfire: a document with both a solid dense score AND a real keyword match would get its dense credit cut to 30% weight, landing it below a zero-evidence document that kept the full 70% dense weight simply because it had no sparse hit to trigger the flip — i.e. more confirming evidence produced a lower score. Verified live: querying "SST" with the old logic, "School House System Implementation" (real dense score + real keyword match, weighted score 0.638) ranked below "Peer Learning Storybook" (zero keyword evidence, weighted score 0.641) — the exact zero-evidence noise pattern this whole fix was meant to demote, still winning against a document with strictly more evidence. Fix: compute both the normal blend and the flipped blend per point, keep whichever is higher. Guarantees a real keyword match can only raise a point's score, never lower it relative to the pre-reweight baseline. Verified live: same School House System vs. Peer Learning Storybook case now keeps School House System's score unchanged at its already-higher flipped value (0.638) rather than degrading further; swept the never- punished invariant (score >= 0.7*dense + 0.3*sparse for any point with a real keyword hit) across 40 live results for "SST" with zero violations. Does not fully fix documents with zero evidence still ranking too high in weak candidate pools (a separate, deeper normalization issue) -- this change only guarantees real evidence is never penalized for existing. Full test suite: 131 passed, 1 pre-existing unrelated failure (Redis cache-clear mock handling, fails identically without this change). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.