mirror of
https://github.com/VectifyAI/PageIndex.git
synced 2026-07-18 21:21:05 +02:00
fix: ceiling semaphore permit leak under cancellation
asyncio.to_thread(ceiling_sem.acquire) blocks a worker thread that can't be interrupted. If the awaiting coroutine is cancelled (Ctrl-C, an outer timeout) while that thread is still parked inside acquire(), the thread can go on to actually acquire the permit after the coroutine has already unwound — leaking it forever, since the matching finally: release() never runs for that attempt. Poll with the non-blocking acquire(False) form instead, which returns instantly and closes the leak window entirely.
This commit is contained in:
parent
e12495dd5b
commit
214098f489
2 changed files with 47 additions and 6 deletions
|
|
@ -103,11 +103,18 @@ async def _llm_semaphore():
|
||||||
node at once and exhausts the process file-descriptor limit (Errno 24).
|
node at once and exhausts the process file-descriptor limit (Errno 24).
|
||||||
"""
|
"""
|
||||||
ceiling_sem = _process_ceiling_semaphore()
|
ceiling_sem = _process_ceiling_semaphore()
|
||||||
# threading.Semaphore.acquire() blocks the calling thread, so run it off
|
# A blocking ceiling_sem.acquire() run via asyncio.to_thread() would be
|
||||||
# the event loop thread — otherwise it would freeze every other coroutine
|
# unsafe under cancellation: the worker thread can't be interrupted, so if
|
||||||
# on this loop while waiting for a slot. release() is non-blocking and
|
# this coroutine is cancelled (Ctrl-C, an outer timeout) while the thread
|
||||||
# safe to call directly from any thread.
|
# is still parked inside acquire(), the thread can go on to actually
|
||||||
await asyncio.to_thread(ceiling_sem.acquire)
|
# acquire a permit *after* we've already unwound — leaking it forever,
|
||||||
|
# since the matching finally: release() below never runs for that attempt.
|
||||||
|
# Poll with the non-blocking form instead: each check returns immediately
|
||||||
|
# (no OS-level wait), so it's safe to call straight from the event loop
|
||||||
|
# thread and there's no window for a background acquire to succeed after
|
||||||
|
# we've already given up on it.
|
||||||
|
while not ceiling_sem.acquire(False):
|
||||||
|
await asyncio.sleep(0.05)
|
||||||
try:
|
try:
|
||||||
effective = get_max_concurrency()
|
effective = get_max_concurrency()
|
||||||
ceiling = _process_wide_max_concurrency()
|
ceiling = _process_wide_max_concurrency()
|
||||||
|
|
|
||||||
|
|
@ -15,7 +15,7 @@ from pageindex.config import (
|
||||||
set_llm_params,
|
set_llm_params,
|
||||||
set_max_concurrency,
|
set_max_concurrency,
|
||||||
)
|
)
|
||||||
from pageindex.index.utils import _llm_semaphore, llm_acompletion
|
from pageindex.index.utils import _llm_semaphore, _process_ceiling_semaphore, llm_acompletion
|
||||||
|
|
||||||
|
|
||||||
@pytest.fixture(autouse=True)
|
@pytest.fixture(autouse=True)
|
||||||
|
|
@ -91,6 +91,40 @@ def test_llm_semaphore_is_a_true_process_wide_ceiling_across_threads():
|
||||||
assert state["peak"] == 3
|
assert state["peak"] == 3
|
||||||
|
|
||||||
|
|
||||||
|
def test_llm_semaphore_cancellation_while_waiting_does_not_leak_a_permit():
|
||||||
|
# A blocking ceiling_sem.acquire() run via asyncio.to_thread() would leak a
|
||||||
|
# permit under cancellation: the worker thread can't be interrupted, so if
|
||||||
|
# the awaiting coroutine is cancelled while the thread is still parked
|
||||||
|
# inside acquire(), the thread can go on to actually acquire the permit
|
||||||
|
# *after* the coroutine already unwound, and the matching finally:
|
||||||
|
# release() never runs for that attempt. Cancel a task waiting on an
|
||||||
|
# already-exhausted ceiling and confirm the permit count fully recovers.
|
||||||
|
set_max_concurrency(1)
|
||||||
|
|
||||||
|
async def run():
|
||||||
|
async def hold():
|
||||||
|
async with _llm_semaphore():
|
||||||
|
await asyncio.sleep(10)
|
||||||
|
|
||||||
|
holder = asyncio.create_task(hold())
|
||||||
|
await asyncio.sleep(0.1) # let it acquire the single permit
|
||||||
|
|
||||||
|
waiter = asyncio.create_task(_llm_semaphore().__aenter__())
|
||||||
|
await asyncio.sleep(0.1) # let it start waiting for the permit
|
||||||
|
waiter.cancel()
|
||||||
|
with pytest.raises(asyncio.CancelledError):
|
||||||
|
await waiter
|
||||||
|
|
||||||
|
holder.cancel()
|
||||||
|
with pytest.raises(asyncio.CancelledError):
|
||||||
|
await holder
|
||||||
|
|
||||||
|
await asyncio.sleep(0.2) # give any orphaned acquire a chance to land
|
||||||
|
assert _process_ceiling_semaphore()._value == 1
|
||||||
|
|
||||||
|
asyncio.run(run())
|
||||||
|
|
||||||
|
|
||||||
def test_llm_semaphore_uses_scoped_override():
|
def test_llm_semaphore_uses_scoped_override():
|
||||||
# A per-index max_concurrency_scope active when the loop's semaphore is first
|
# A per-index max_concurrency_scope active when the loop's semaphore is first
|
||||||
# created must set its size, and must not mutate the process default.
|
# created must set its size, and must not mutate the process default.
|
||||||
|
|
|
||||||
Loading…
Add table
Add a link
Reference in a new issue