Aller au contenu
login
arrow_backRetour aux issues
phasespace-labs/palinode #142

Reindex runs on the event loop and freezes every other API endpoint

ecoDébutant bug good first issue

descriptionDescription

**If you'd like to take this, please comment here first and wait to be assigned before opening a PR.** Two contributors landed on the same issue once because we had no such line; that was our fault, and this is the fix. `POST /reindex` is declared `async def`, but everything it does is synchronous. So it occupies the event loop for the entire reindex, and every other endpoint — `/search`, `/health`, `/save`, and every MCP tool that proxies through the API — waits for it to finish. **Where it is**, all in `palinode/api/routers/maintenance.py`: | Line | What is there | |---|---| | 87 | `async def reindex_api(...)` | | 99 | `if _reindex_lock.locked():` — the 409 guard | | 122 | `async with _reindex_lock:` | | 133 | `for filepath in files:` | | 138 | `handler._process_file(filepath)` — file read + embedder HTTP call + sqlite writes, all blocking | Every other hot handler in `palinode/api/routers/search.py` and `memory.py` is a plain `def`, which is why FastAPI runs them in its threadpool and they don't have this problem. This handler is the one exception, and the only reason it is `async` at all is the lock it takes: `palinode/api/_util.py:80` — `_reindex_lock = asyncio.Lock()`. **The fix:** make the handler a plain `def` and make the lock a `threading.Lock`. FastAPI will then run it in the threadpool like the others. ### The part that is easy to get wrong **Do not keep the `.locked()`-then-acquire shape.** Today's code checks `_reindex_lock.locked()` at line 99 and acquires at line 122, and that is safe *only by accident*: there is no `await` anywhere between those two lines, so the coroutine cannot be suspended in the gap, and on a single event loop nothing else can interleave. Running the handler in the threadpool removes exactly that guarantee. Two requests would then be able to **both** see `locked() == False`, and the second would block at the `with` statement and run a **second full reindex** the moment the first finished — instead of returning 409. That is worse than the bug being fixed here. **Acquire atomically and let the acquire itself be the guard:** ```python if not _reindex_lock.acquire(blocking=False): raise HTTPException( status_code=409, detail="reindex already running — check /status for progress", ) try: ... # the whole reindex body finally: _reindex_lock.release() ``` `acquire(blocking=False)` returns `False` rather than waiting, so the 409 path and the mutual exclusion become one atomic operation. The `finally` matters: without it a raised exception leaves the lock held and every later reindex returns 409 forever. **Full scope — this is all of it:** 1. `palinode/api/routers/maintenance.py:87` — `async def` → `def` 2. `palinode/api/routers/maintenance.py:99` — replace the `if _reindex_lock.locked():` guard with the non-blocking acquire above 3. `palinode/api/routers/maintenance.py:122` — `async with _reindex_lock:` → `try:` / `finally: _reindex_lock.release()`, since the acquire has already happened at the guard 4. `palinode/api/_util.py:80` — `asyncio.Lock()` → `threading.Lock()`, plus the `import threading` 5. `palinode/api/_util.py:76-78` — the comment above the lock explains the *current* design ("`asyncio.Lock` is safe because FastAPI runs on a single event loop…"). It becomes wrong with this change; please rewrite it to say what now guarantees exclusion. 6. `tests/test_reindex_concurrency.py:108` — `async with srv._reindex_lock:` must become a plain `with`. That test currently spins up a **second event loop in a background thread** just to hold an async lock; with a threading lock the helper gets considerably simpler, and simplifying it is part of this issue. 7. `tests/test_reindex_concurrency.py:83` — patches `.locked()`. With the guard no longer calling `.locked()`, this test needs rewriting to hold the real lock instead of patching the predicate. `palinode/api/server.py:47` re-exports `_reindex_lock` so that tests can reach
codeOuvre sur GitHub