fix: resolve concurrency and DoS pitfalls from PR #272 review

- index/utils.py: fix an asyncio deadlock in _sync_llm_semaphore. Sync LLM
  calls (check_toc / process_no_toc / toc_transformer) run on the event-loop
  thread nested inside the async meta_processor, and its blocking ceiling
  acquire could wait forever for a permit held by async _llm_semaphore holders
  that can only release it once the (now-frozen) loop runs. Take the slot
  non-blocking when on a running loop; keep the blocking acquire off-loop.
- index/utils.py: bound parse_pages ranges before materializing range() into
  the list, so a huge span like '1-2000000000' is rejected up front instead of
  exhausting memory before the 1000-page cap is ever checked (DoS).
- storage/sqlite.py: bump a generation counter on close() so a thread that
  cached a connection in thread-local storage reconnects on its next call
  instead of reusing a closed handle (ProgrammingError).
- backend/cloud.py: after connect, bail out of the SSE background thread if the
  consumer already abandoned the stream, instead of draining it in the background.

Adds regression tests for each fix.
This commit is contained in:
mountain 2026-07-10 14:49:55 +08:00
parent d97231b480
commit 9ad54122bb
6 changed files with 192 additions and 10 deletions

View file

@ -290,6 +290,56 @@ def test_llm_completion_holds_the_shared_semaphore(monkeypatch):
assert state["peak"] == 1
def test_sync_llm_completion_on_event_loop_does_not_deadlock(monkeypatch):
# Regression: _sync_llm_semaphore used a BLOCKING ceiling acquire. Sync LLM
# helpers (check_toc, process_no_toc, toc_transformer, …) run synchronously
# ON the event loop (nested inside the async meta_processor). If async
# llm_acompletion holders occupy every ceiling permit across their awaits,
# a blocking acquire froze the loop -> the holders could never resume to
# release their permits -> permanent deadlock. The sync path must never
# block the running loop.
set_max_concurrency(2)
async def fake_acompletion(**kwargs):
await asyncio.sleep(0.3) # hold a ceiling permit across the await
return SimpleNamespace(
choices=[SimpleNamespace(message=SimpleNamespace(content="ok"))]
)
def fake_completion(**kwargs):
return SimpleNamespace(
choices=[SimpleNamespace(
message=SimpleNamespace(content="sync-ok"), finish_reason="stop")]
)
monkeypatch.setattr("litellm.acompletion", fake_acompletion)
monkeypatch.setattr("litellm.completion", fake_completion)
async def run():
# Both ceiling permits taken by async holders, held across their await.
holders = [asyncio.create_task(llm_acompletion("m", f"p{i}")) for i in range(2)]
await asyncio.sleep(0.05) # let them acquire the permits
# Sync call on the loop thread: pre-fix this blocks forever waiting for a
# permit the holders own and can't release (loop is frozen).
result = llm_completion("m", "sync")
await asyncio.gather(*holders)
return result
# Run in a thread with a join timeout so a regression FAILS instead of
# hanging CI: a real deadlock freezes the loop, so asyncio.wait_for can't
# cancel it (its timeout callback never runs on the frozen loop).
box = {}
def target():
box["result"] = asyncio.run(run())
t = threading.Thread(target=target, daemon=True)
t.start()
t.join(timeout=8)
assert not t.is_alive(), "deadlock: sync llm_completion blocked the event loop"
assert box["result"] == "sync-ok"
def test_run_async_propagates_scope_into_worker_thread():
# When build_index runs inside an already-running loop, _run_async hops to a
# worker thread. The max_concurrency_scope override must ride along (copied

View file

@ -50,6 +50,31 @@ def test_retrieve_parse_pages_delegates_to_canonical_and_enforces_dos_cap():
_parse_pages("1-99999999")
def test_parse_pages_caps_a_huge_range_without_materializing_it():
"""Regression: the 1000-page cap was checked only AFTER
`result.extend(range(start, end + 1))`, so a single huge span like
'1-2000000000' allocated billions of ints and OOM'd before the check ran.
The span must be rejected up front, quickly, without building the list."""
import time
import pytest
from pageindex.index.utils import parse_pages
start = time.monotonic()
with pytest.raises(ValueError, match="too large"):
parse_pages("1-2000000000")
# Must be near-instant (no billion-element allocation). Generous bound to
# avoid flakiness while still failing loudly on a re-materializing regression.
assert time.monotonic() - start < 1.0
# Boundary: exactly 1000 pages is allowed; 1001 is rejected.
assert parse_pages("1-1000") == list(range(1, 1001))
with pytest.raises(ValueError, match="too large"):
parse_pages("1-1001")
# A range that fits but whose accumulation across parts crosses the cap.
with pytest.raises(ValueError, match="too large"):
parse_pages("1-600,700-1400")
def test_retrieve_get_pdf_page_content_falls_back_to_canonical(tmp_path, monkeypatch):
"""When no cached 'pages' are present, the file-read fallback must
delegate to the canonical get_pdf_page_content instead of re-implementing

View file

@ -139,6 +139,46 @@ def test_close_closes_connections_created_in_other_threads(storage):
conns["worker"].execute("SELECT 1")
def test_worker_reconnects_via_get_conn_after_close(storage):
"""Regression: after close(), a thread that had already cached a connection
in thread-local storage would get that now-CLOSED handle back from
_get_conn (close() can only del its own thread-local), raising
ProgrammingError instead of transparently reconnecting. A generation bump
on close() must make the SAME thread's next _get_conn hand back a fresh,
working connection."""
import threading
storage.create_collection("papers")
cached = threading.Event()
closed = threading.Event()
result = {}
def worker():
# 1. cache a connection in this thread's thread-local
storage._get_conn().execute("SELECT 1")
cached.set()
# 2. wait until the main thread closed the storage (invalidating it)
closed.wait(timeout=5)
# 3. reuse from the SAME thread -> must reconnect, not reuse closed conn
try:
result["val"] = storage._get_conn().execute("SELECT 1").fetchone()[0]
result["list"] = storage.list_collections()
except Exception as e: # noqa: BLE001 - record for assertion
result["err"] = f"{type(e).__name__}: {e}"
t = threading.Thread(target=worker)
t.start()
cached.wait(timeout=5)
storage.close() # closes + invalidates the worker's cached connection
closed.set()
t.join(timeout=5)
assert "err" not in result, f"reconnect after close failed: {result.get('err')}"
assert result["val"] == 1
assert result["list"] == ["papers"]
def test_duplicate_file_hash_in_collection_raises(storage):
"""UNIQUE(collection_name, file_hash) guards the add-same-file race."""
import sqlite3