Skip to content

fix(personal): run directory indexing off the event loop#5634

Open
StressTestor wants to merge 1926 commits into
devfrom
fix/add-directory-event-loop
Open

fix(personal): run directory indexing off the event loop#5634
StressTestor wants to merge 1926 commits into
devfrom
fix/add-directory-event-loop

Conversation

@StressTestor

Copy link
Copy Markdown
Collaborator

Summary

POST /api/personal/add_directory is an async def handler that called the fully synchronous rag.index_personal_documents inline, so the entire indexing job (os.walk, file reads, per-chunk embedding, Chroma inserts) ran on the event loop. Every other request queued behind it: indexing a real directory froze the UI and API for 25+ minutes with no sign of life (the reporter's users concluded the app crashed).

This PR moves the blocking section into the threadpool via fastapi.concurrency.run_in_threadpool. personal_docs_manager.add_directory stays inside the moved section because it triggers refresh_index(), which re-extracts text across tracked directories, also blocking work. A module-level lock serializes index jobs: without it the threadpool move would let concurrent requests run parallel jobs that race PersonalDocsManager's unsynchronized list mutations and plain open('w') file writes. Jobs previously serialized on the blocked loop anyway, so one-at-a-time is behavior parity, minus the freeze.

Target branch

  • This PR targets dev, not main.

Linked Issue

Fixes #5558

Type of Change

  • Bug fix (non-breaking — fixes a confirmed issue)

Checklist

  • I searched open issues and open PRs — this is not a duplicate.
  • This PR targets dev
  • My changes are limited to the scope described above — no unrelated refactors or whitespace changes mixed in.
  • I actually ran the app (uvicorn app:app + a local ChromaDB at localhost:8100) and verified the change works end-to-end.

How to Test

  1. python -m pytest tests/test_add_directory_event_loop.py: 4 tests. The thread-identity test asserts indexing and bookkeeping run off the event loop thread (fails on dev, where both run on it); the serialization test asserts concurrent requests never run parallel index jobs; the other two pin response shape and the error path.
  2. End-to-end (what i ran): start ChromaDB and the app, seed data/personal_docs/bigvault/ with 150 markdown files, POST /api/personal/add_directory, and while the job is running, time a GET /api/personal from a second terminal.
  3. Results on this branch: the GET answers in 0.002s while indexing is in flight. Same probe on unpatched dev: the GET takes 3.2s, because it queues behind the whole remaining index job (with the reporter's 25-minute job, that GET waits minutes).
  4. The index job itself completes identically on both: indexed_count: 150, failed_count: 0.

Also ran: full python -m pytest (4674 passed, 3 skipped; the 5 failures are the docker-socket tests, which fail identically on clean dev on this machine, macOS with no docker socket) and python -m compileall app.py core routes src services scripts tests.

Scope notes

  • This is the issue's option 2 (threadpool), not option 3 (background job with id + progress). The request still blocks the client for the duration, but the app stays responsive for everyone. Option 3 is a feature-sized change (new endpoints + Knowledge UI work); happy to take it as a follow-up if wanted.
  • A long index job still holds the HTTP request open, so aggressive proxy timeouts can drop the response while the job completes server-side. Pre-existing behavior, unchanged here.
  • The lock covers add_directory jobs only. Now that the app stays responsive during indexing, an admin hitting POST /reload or DELETE /remove_directory mid-job could race the indexing thread's bookkeeping, a window that could not occur before because the blocked loop prevented any dispatch. Admin-only and narrow; the clean fix is a lock inside PersonalDocsManager itself, which i kept out of this PR to keep the diff reviewable. Can follow up.

pewdiepie-archdaemon and others added 30 commits June 27, 2026 21:10
Keep an unhealthy MemoryVectorStore instance available for health reporting instead of discarding it as disabled. This lets health checks report a degraded/down vector-store state while preserving focused regression coverage for initializer behavior.
falabellamichael and others added 25 commits July 11, 2026 15:14
…ls (#5420)

* fix: harden stabilization attachment and agent guards

* fix(uploads): preserve durable references during cleanup

* fix(uploads): close cleanup and compaction races
…4411) (#5160)

* docs: update static/js/MODULE_SUMMARY.md to reflect current ES6 frontend

Rewrite the stale module summary to match the current no-build,
ES6-module frontend architecture. Adds coverage of app.js orchestration,
the chat/SSE pipeline (chat.js, chatStream.js, chatRenderer.js,
streamingRenderer.js), new subsystems (research/, compare/, document
streaming, cookbook*, skills.js), and removes the obsolete <script> load
order assumptions.

* cleanup: remove dead MEMORY_DOC / memory_doc paths (closes #4411)

Removes the unused MEMORY_DOC constant and the matching DataConfig
memory_doc field / set_data_paths entry. No runtime code imports or
references these paths, so this is a no-behavior-change dead-code
cleanup under the storage-architecture tracker #4377.
* fix(db): restrict data/app.db to 0600

app.db holds bearer-token hashes, bcrypt password hashes, and encrypted
provider keys but was created under the default umask (0644 -> world-readable),
unlike .app_key/vault/integrations which are already 0600 via safe_chmod.

init_db() now chmods the SQLite file to 0600 right after create_all (POSIX
only; no-op on Windows, skipped for Postgres / in-memory). Unconditional and
idempotent, so it also re-locks already-deployed 0644 installs on next
startup. The transient rollback journal inherits 0600 from the parent file at
creation - no sidecar handling needed; -wal/-shm don't exist until WAL is
enabled (#4409 C4) and inherit the same mode then.

Satisfies Rule B, unblocking #4413 and the vault/integration secret moves.
Mirrors src/secret_storage.py:43-45.

Verified: security + DB-permission suites pass; 6 pre-existing visual_report
failures (missing markdown/nh3 deps) are unrelated.

Closes #4407

* fix(db): harden SQLite path parsing and re-lock sidecars

Address review feedback on #4420.

P2: derive the file to chmod from engine.url (SQLAlchemy's parsed URL)
via _sqlite_db_path(), instead of DATABASE_URL.replace("sqlite:///", "").
A driver-qualified URL (sqlite+pysqlite://) or one carrying query args
(?cache=shared) previously slipped past the prefix check / string slice
and left the DB world-readable; the parsed path resolves correctly and
drops the query.

P3: re-lock stale -wal/-shm/-journal sidecars to 0o600 at startup. The
main file is chmod'd first, so any sidecar SQLite creates afterward
inherits 0o600, but a -wal/-shm left world-readable by an older 0o644
install (once WAL was enabled) could still expose DB pages. Absent
sidecars are the normal case, not an error.

Tests: unit-test _sqlite_db_path across driver/query/memory/postgres URL
forms, and a subprocess test asserting stale 0o644 -wal/-shm are
re-locked on startup.

* fix(db): handle sqlite file URI app db permissions

* fix(db): close remaining SQLite permission bypasses

---------

Co-authored-by: Ethan <23321960+0xLeathery@users.noreply.github.com>
Co-authored-by: Alexandre Teixeira <alexandremagteixeira@gmail.com>
chat_stream() references `_explicit_web_intent` in three places
(disabled-tools gating, global-disabled web allowance, and the
per-turn tool filter) but the assignment was dropped during a
branch merge. Every chat request raised

    NameError: name '_explicit_web_intent' is not defined

at routes/chat_routes.py, surfacing to the client as a bare
"Internal Server Error" before any LLM call was made — chat was
fully broken on dev and main.

Restore the original definition, computed from the already-derived
tool intent, immediately before its first use:

    _explicit_web_intent = bool(_tool_intent and _tool_intent.category == "web")

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
(cherry picked from commit 2d81770)
…mporter (#5261)

* Harden skill importer against SSRF: block private targets + revalidate redirects per hop

The skill importer validated only the initial URL with the lenient SSRF guard
(block_private=False) and then fetched with follow_redirects=True, so a 3xx to
an internal/metadata address (169.254.169.254, 127.0.0.1, RFC-1918) was still
connected to — inconsistent with the hardened services/search/content.py
:_get_public_url path.

Add a _get_checked() helper that follows redirects manually and re-runs the
SSRF guard with block_private=True on every hop, and route all three fetch
sites (skills.sh unwrap, _fetch_bytes, _list_github_dir) through it. GitHub's
own redirects and the final-host _assert_github_url checks are preserved.

Adds hermetic regression tests (IP-literal hosts, faked HTTP layer) and updates
the existing mock signature for the new block_private kwarg.

Defense-in-depth: the endpoint is admin-gated (require_admin) and admins are
trusted per THREAT_MODEL.md, so this is not a cross-boundary vulnerability.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* test: enforce follow_redirects=False invariant in mock client

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Add Arch-specific package installation and NVIDIA runtime configuration
to the Docker setup guide. Cover passthrough verification, the NVIDIA
Compose overlay, and the distinction between GPU passthrough and
CUDA-backed model serving

Refs #831
chore: sync upstream changes from odysseus-dev/odysseus
fix(docker): bump Docker CLI to a patched release
fix(docker): bump Docker CLI to a patched release
* feat(models): define capability schema and readers

* fix(models): harden Google catalog probing

Restrict native catalog probing to the Gemini host, keep provider keys out of request URLs, filter non-chat model resources, and preserve the manual refresh default in the built-in Google add flow.
…5474)

* security(url-safety): reject RFC 6598 shared address space in strict mode

Strict mode (block_private=True) is a full SSRF lockdown, but it only
rejected is_private and is_loopback targets. CPython does not classify RFC
6598 shared/CGNAT space (100.64.0.0/10) as is_private (it is "shared", not
"private"), so a public redirect into 100.64.0.1 passed the per-hop guard
and still issued the request to a potentially internal CGNAT service.

not is_global would also exclude it, but only on CPython 3.11.10+/3.12.4+/
3.13+; the CI matrix runs 3.11/3.12, so reject the range explicitly to stay
correct across patch levels and the 3.14 runtime image. Default local-first
mode is unchanged. Adds strict-mode coverage for shared, non-global, and
public targets.

* docs(url-safety): correct CGNAT is_global rationale in strict-mode comment

The prior comment claimed `not is_global` catches 100.64.0.0/10 only on
CPython 3.11.10+/3.12.4+/3.13+. That is inaccurate for CGNAT: is_global
is False for 100.64.0.1 on every supported version (verified 3.10-3.14).
The version-fragility applies to other ranges gh-113171 touched, not CGNAT.
The explicit range reject is still the right choice; restate the reason as
is_private not covering shared space, and not coupling strict mode to
is_global's broader, cross-version definition. No behavior change.
#5491)

* fix(llm): enhance fallback logic to handle empty completions and improve metadata handling

* fix(llm): stream tool call deltas immediately
Slice 2f of the route-domain reorganization (#4082/#4071, per
specs/architecture-runtime-inventory.md §6.3). Moves note_routes.py into
routes/note/, leaving a backward-compat sys.modules shim at the old path.
Pure file reorganization, no behavior change.

The shim uses sys.modules replacement (same pattern as the merged gallery
#4903, research #4975, memory #5007, history #5090, and contacts #5227
slices) so that `import routes.note_routes`, `from routes.note_routes import
X`, `importlib.import_module(...)`, and the `import ... as note_routes` +
`monkeypatch.setattr(note_routes, "SessionLocal", ...)` pattern used by
test_note_reminder_fire_scope.py / test_notes_fail_closed_auth.py all
operate on the same module object the application uses.

The canonical module does NOT depend on the shim — routes/note/note_routes.py
imports only from core/, src/, and stdlib. The outbound email cross-domain
imports (routes.email_routes._get_email_config, routes.email_helpers.
_send_smtp_message) are function-local lazy imports that keep resolving
through the email module's own path (email is not yet migrated).

One source-introspection test site repointed to the new canonical path:
- test_model_helper_owner_scope.py (shared with history; history entry
  already repointed in #5090, note entry repointed here)

Adds tests/test_note_routes_shim.py to pin the sys.modules shim contract
(legacy and canonical paths resolve to the same module object; monkeypatch
via legacy alias reaches the canonical module).

Verified: compileall clean; full suite 4487 passed, 3 skipped.
POST /api/personal/add_directory called rag.index_personal_documents
inline from an async handler, so the whole indexing job (os.walk, file
reads, per-chunk embedding, Chroma inserts) ran on the event loop and
every other request queued behind it. Indexing a real directory froze
the UI and API for 25+ minutes with no sign of life.

Move the blocking section into the threadpool via run_in_threadpool.
personal_docs_manager.add_directory stays inside it because its
refresh_index() re-extracts text across tracked directories, which is
also blocking work. A module-level lock serializes index jobs so the
threadpool move does not introduce parallel jobs racing
PersonalDocsManager's unsynchronized list mutations and file writes;
they previously serialized on the blocked loop, so one-at-a-time is
behavior parity.
@github-actions github-actions Bot added the ready for review Description complete — ready for maintainer review label Jul 20, 2026
@RaresKeY

Copy link
Copy Markdown
Member

I checked the latest head against current dev. The single-request offload works, but its admission boundary still permits inconsistent index state and broad request stalls under overlap.

Findings

P2 Badge issue (concurrency): Admit and serialize directory mutations before offloading

  • Problem: _index_job_lock is acquired only after an add has consumed a shared AnyIO worker token, and remove does not take it at all. One add/remove interleaving returned 200 for both requests but left tracking and vectors in a state no complete serial order can produce; separately, 40 queued adds borrowed all 40 default worker tokens and prevented an unrelated synchronous endpoint from starting.

  • Impact: A successful removal can leave the directory re-tracked or its content partially searchable, while a burst or retry loop of authorized add requests can recreate the multi-minute application hang for synchronous routes and dependencies sharing the worker pool.

  • Ask: Acquire asynchronous serialization admission before offloading an add, and run remove's complete tracking/vector transition through the same gate. Audit reload/upload/file/direct mutation paths against that boundary, keep admitted blocking work off the event loop, and add same-loop add-vs-remove plus worker-capacity regressions.

  • Location: routes/personal_routes.py:220

Validation

  • All exact-head required GitHub checks are green, including pytest and compileall.
  • Focused and adjacent personal/RAG tests passed, and compile/diff checks are clean.
  • Independent secretless route probes reproduced both impossible add/remove final states and the 40-token worker-capacity exhaustion.
  • Not run: live Chroma/embedding concurrency, a real 25-minute directory load, multiple server processes, or real client-disconnect/proxy behavior.

The #5558 fix took the job lock INSIDE the threadpool worker and only on the
add path, so (1) remove_directory and /reload mutated PersonalDocsManager's
unsynchronized list/index concurrently with an in-flight add — the inconsistent
state the PR claimed to prevent — and (2) a queued add blocked on the lock while
holding an AnyIO threadpool token, starving the shared pool.

Move the lock to an asyncio.Lock acquired in the async handler BEFORE offloading,
and route add, remove and reload through it. A waiting request now parks on the
event loop instead of pinning a worker, and all three mutators are serialized so
the 'add/remove are serialized and cannot leave inconsistent state' guarantee
holds. remove and reload also run their blocking work off the event loop. The
lock is per-router so each app binds it to its own loop; single-process scope.

Tests: add-vs-remove and add-vs-reload serialization regressions (async via
ASGITransport, since asyncio.Lock deadlocks starlette TestClient's portal); the
existing add-vs-add test converted to the same driver.
@StressTestor

Copy link
Copy Markdown
Collaborator Author

pushed 13c7f6d. moved the job lock to an asyncio.Lock acquired in the handler before offloading, and routed add, remove, and reload through it. that closes both halves: remove/reload are now serialized against an in-flight add (the inconsistent-state path), and a queued request parks on the event loop instead of pinning a threadpool worker (the starvation). remove and reload also run their blocking work off the loop now.

added add-vs-remove and add-vs-reload serialization regressions, and converted the existing add-vs-add test to the same driver. they run async via ASGITransport because an asyncio.Lock deadlocks the sync TestClient portal (same reason test_notes_fail_closed_auth.py uses it). left the upload/delete sibling handlers and the partial-index-on-bookkeeping-failure window as separate follow-ups to keep this scoped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready for review Description complete — ready for maintainer review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add_directory indexes synchronously on the event loop — entire app unresponsive for the duration (25+ min observed)