Fix web UI slowness: delete drain starved the app, janitor race stranded tombstones#7
Merged
Merged
Conversation
…ded tombstones
Loading and deleting a project from the web UI were both very slow, and the
delete modal could sit on "Deleting..." indefinitely. All three symptoms came
from the delete path, and none from the code the symptoms pointed at: GET
/projects/{id} measures 49-273ms on an idle process.
Two independent bugs:
1. The batched drain starved the whole interpreter. _drop_chroma_collection
yielded a FIXED time.sleep(0.01) between batches that each held the GIL for
~500ms (chromadb 1.x rust calls hold it for the whole call). Batching bounds
one stall; only the gap controls the duty cycle, and the gap has to scale
with the hold. Everything else in the process -- every HTTP handler, the SSE
streams, the job worker -- ran at ~5% for the duration of a drain.
2. The startup janitor raced the cleanup that owned the collection.
_warm_vectorstore passed db.list_projects(), which EXCLUDES 'deleting', so a
tombstoned project's live collections were classified as orphans and drained
a second time, concurrently with the _cleanup_deleted thread that
_recover_on_restart had just spawned. Whichever thread reached
delete_collection first made the other's next get() raise; that was reported
as "interrupted", and _cleanup_deleted returned BEFORE removing the data dir
and BEFORE db.delete_project. The tombstone survived and was re-drained on
every boot, so the app was permanently in delete-recovery -- which is why
loading a project was slow even when nothing was being deleted.
Fixes:
* vectorstore: yield proportionally to each batch's own measured hold; DROP_BATCH
500 -> 200 to bound the worst-case stall; refuse a second concurrent drain of
one collection; treat "collection already gone" as success rather than
interruption; thread `cancel` through drop_orphan_collections so a startup
sweep no longer ignores Ctrl+C.
* main: include 'deleting' projects in the janitor's "known" set (the same union
_sweep_orphan_project_dirs already used), and announce pending reclaims at
startup instead of failing silently.
* workspace_service/db: count_file_index() instead of materializing every row to
take len() (36ms -> <1ms), and count collections without get_or_create -- so
merely viewing a project stops CREATING its cases collection as a side effect
of a read.
* jobs: Terminate's drain now takes a cancel callback, so Ctrl+C during a large
Terminate no longer waits out the whole drain.
Measured on a copy of a real 2.1 GB store (79,585-chunk collection):
p95 request stall during delete 823ms -> 170ms
worst-case stall 1480ms -> 359ms
app availability 5% -> 38%
drain wall-clock 131s -> 157s
tests/verify_delete_race.py pins all three defences. It is verified to FAIL if
any one of them is reverted individually -- an earlier version of the test
passed against the buggy code because it relied on hitting the race by chance,
so the interleaving is now forced rather than hoped for.
This does not replace the drain with a one-shot delete_collection: measured at
51.7s of SOLID GIL hold (2 scheduler ticks in 51.7s), and chroma's sqlite runs
in rollback-journal mode, so a kill mid-way rolls the whole thing back -- the
exact failure the batched drain was introduced to remove.
…and fast
CI's full gate failed because scripts/run_acceptance.py enforces that every
tests/verify_*.py appears in its SUITE manifest -- so a new test cannot go
silently unrun -- and the new script was not registered. Registered as CORE.
Two changes were needed before it belonged in a gate rather than in the LOCAL
tier alongside verify_delete_responsive:
* Removed the wall-clock assertions. The in-flight guard was proven by timing a
second drain call ("returned in <1s while the first was still running"), which
is a scheduling race on a shared runner. It now occupies the collection name
directly and asserts the refusal with no threads and no clock; a separate,
timing-free check covers that a real drain claims and releases the name.
Likewise the "collection vanished mid-drain" case is now forced with a stub
collection that reports rows once and then raises NotFound, instead of racing
a second thread to delete it at the right moment.
* Fixed a wait condition that made the script take 184s. The end-to-end section
polled `not db.list_projects("deleting")`, but section 1 deliberately leaves
its own tombstone in place, so that never became true and the loop burned its
full 180s timeout on every run. It now waits on the project under test only.
Runtime: 184s -> 4.5s. Fixture sizes trimmed to match (3000 -> 1200 chunks).
Re-validated after the rewrite: reverting each of the three defences
individually still fails its own check (24/26, 25/26, 25/26 respectively).
Full core tier green locally: 24 passed, 0 failed, 0 skipped.
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
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.
What was wrong
Loading a project and deleting a project from the web UI were both very slow, and the delete modal could sit on "Deleting…" indefinitely.
All three symptoms trace back to the delete path — and none to the code they point at.
GET /projects/{id}measures 49–273 ms on an idle process, so the load path was never the problem. The app was simply GIL-starved from boot.Bug 1 — the batched drain starved the whole interpreter
_drop_chroma_collectionyielded a fixedtime.sleep(0.01)between batches that each held the GIL ~500 ms (chromadb 1.x rust calls hold it for the entire call).Batching bounds a single stall; only the gap controls the duty cycle, and the gap has to scale with the hold. With a 10 ms gap against a 500 ms hold, every other Python thread — all HTTP handlers, the SSE streams, the job worker — got ~5% of the interpreter for the whole drain.
Bug 2 — the startup janitor raced the cleanup that owned the collection
_warm_vectorstorepasseddb.list_projects(), which excludes'deleting'. So a tombstoned project's live collections were classified as orphans and drained a second time, concurrently with the_cleanup_deletedthread_recover_on_restarthad spawned one line earlier in lifespan.Whichever thread reached
delete_collectionfirst made the other's nextget()raise. That was reported as "interrupted", and_cleanup_deletedreturned before thermtreeand beforedb.delete_project. The tombstone survived and was re-drained on every boot — so the app was permanently in delete-recovery, which is why loading a project was slow even when nothing was being deleted.The sibling function four lines up (
_sweep_orphan_project_dirs) already had this right, with the correct rationale in its docstring.Changes
openmind/vectorstore.pyDROP_BATCH500→200 to bound the worst-case stall. Refuse a second concurrent drain of one collection. Treat "collection already gone" as success, not interruption. Threadcancelthroughdrop_orphan_collections.openmind/main.py'deleting'projects in the janitor'sknownset. Announce pending reclaims at startup instead of failing silently.openmind/db.py,services/workspace_service.py,ports/workspace_repository.pycount_file_index()instead of materializing every row to takelen()(36 ms → <1 ms). Count collections withoutget_or_create— so viewing a project stops creating its cases collection as a side effect of a read.openmind/jobs.pyMeasured
On a copy of a real 2.1 GB store (79,585-chunk collection), with a 10 ms-tick thread standing in for the FastAPI threadpool:
The full sweep behind the
DROP_BATCH/DROP_YIELD_RATIOchoice is in a comment table above the constants, so the next person doesn't re-derive it.verify_delete_responsivealso improves on its own existing assertion: max/projectslatency during a drop went 0.21 s → 0.01 s.Testing
tests/verify_delete_race.py(26 checks) pins all three defences.On the new test: my first version passed against the buggy code, because it relied on hitting the race by chance. It was rewritten to force the interleaving deterministically, then validated by reintroducing each of the three fixes' bugs separately and confirming the matching check fails:
main.pyknownset revertedIt calls the real
main._warm_vectorstore()rather than re-deriving itsknownset, so it actually guards the fix rather than a copy of it.Reviewer notes
Upgrade impact: this makes pending deletes complete. Anyone carrying a stranded
'deleting'tombstone will have that project's storage actually reclaimed on the next server start — correct, but irreversible, so it's worth knowing before upgrading. Startup now logs pending reclaims by id.Rejected alternatives, both deliberate:
delete_collection. It is less total work, but measured at 51.7 s of solid GIL (2 scheduler ticks in 51.7 s), and chroma's sqlite runs in rollback-journal mode, so a kill mid-way rolls the whole thing back — precisely the failure the batched drain was introduced to remove (see 0721234).sweep_orphan_segment_dirs.Known follow-up: this is a mitigation — a 79k-chunk delete is still ~157 s of background churn. The measured real fix is a per-project Chroma directory, making delete an
rmtree: 0.03 s with zero GIL impact. I verified several per-projectPersistentClients coexist fine in one process, and that the Windows file-lock teardown needsSharedSystemClient.clear_system_cache()+gc.collect()before thermtree. Left out of this PR because it needs a data migration and touches the ingest/search paths — worth doing as a separate deliberate change.