arrow_backRetour aux issues
phasespace-labs/palinode
#142
Débutant
Ouvrirarrow_forward
Débutant
Ouvrirarrow_forward
Débutant
Ouvrirarrow_forward
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
Issues similaires
calkit/calkit
star53
Poids du dépôt moyen
VS Code extension should be robust to YAML parser errors
Seeing this error: ``` Failed to read calkit.yaml: YAMLParseError: A block sequence may not be used as an implicit map…
Python
bug
good first issue
fu351/Doberman-Core
star211
Poids du dépôt léger
dash: a manual Refresh control
The dashboard polls: `refreshStats()` (`src/doberman/dash/app.py:408`) every 5 s and `refreshPending()` (`:546`) every …
Python
enhancement
good first issue
fu351/Doberman-Core
star211
Poids du dépôt léger
dash: "Copy details" button on each pending-approval card
Each pending-approval card in the dashboard (`renderPending`, `src/doberman/dash/app.py:448-544`) shows the risk badge,…
Python
enhancement
good first issue