Make indexing progress observable and stuck indexes recoverable - #8
Conversation
Two bugs whose failure mode was silence, both from the audit.
1. PROGRESS WAS WRITE-ONLY
IndexingService wrote progress into a per-instance dict. The SSE endpoint builds its own
IndexingService per request, so it never saw those writes and always fell through to the
database branch, which hardcodes current_step="Unknown" and a percentage of 0 or 100. The
per-file progress the indexer computes could not reach any client. A per-process dict
would not fix it either: indexing runs in a background task and, with more than one
worker, in a different process from the reader.
core/progress.py adds a ProgressStore over a capped Redis stream per repository
(progress:{repo_id}, MAXLEN 500) with a process-wide in-memory fallback for the no-Redis
case. A stream rather than a key because the SSE endpoint wants history: a client that
connects late can replay what it missed via XRANGE instead of seeing only the current
value. get_progress now reads the shared store first, then the instance dict, then the
database.
Publishing never raises. Progress is telemetry, and a Redis blip must not fail an
otherwise healthy index, so a publish failure degrades to memory and the caller sees
nothing. There is a test with a Redis double that raises on every call.
2. A KILLED INDEX WAS UNRECOVERABLE
Only FAILED and COMPLETED are terminal, and every self-healing path (repos.py,
seed_demo.py) keys on FAILED -- so a container killed during CLONING, PARSING or EMBEDDING
left the row in that state forever with no way to retry through the API. In demo mode that
bricked the deployment. reap_stuck_indexing runs at startup, where anything still in a
transient state provably has no live indexer because the process that owned it is gone.
PENDING is deliberately left alone: a repository queued but not yet started is not stuck,
and failing it would break the normal import path. An existing indexing_error is
preserved, since a real failure reason beats the generic interruption note.
Also hardened the SSE route, which looped forever with no client check and no ceiling: a
repository stuck mid-index held an open response and its database session indefinitely,
once per browser tab. It now checks request.is_disconnected(), emits only on change
instead of re-sending an identical payload every second, and stops after 15 minutes with
a reconnect hint.
Verified: 17 new tests, 159 total (was 142), ruff clean, run with apps/api/.env moved
aside and OPENAI_API_KEY / AZURE_OPENAI_API_KEY / ANTHROPIC_API_KEY / REDIS_URL all unset.
The Redis paths are exercised with fakeredis, which implements streams, consumer groups
and XAUTOCLAIM, so they are genuinely executed rather than mocked. End-to-end check: a
fresh IndexingService with an empty _progress dict -- the exact object the SSE route
builds -- now returns 57.5% and "Parsing widget.tsx..." where it previously returned
"Unknown" at 0%.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 34c7490240
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "UPDATE repositories SET status = 'FAILED', " | ||
| "indexing_error = COALESCE(indexing_error, " | ||
| "'Indexing was interrupted before completing. Re-import to retry.') " | ||
| f"WHERE LOWER(status) IN ({placeholders})" |
There was a problem hiding this comment.
In deployments that use PostgreSQL for DATABASE_URL, SQLEnum(IndexingStatus) is a native enum column, and LOWER(status) is not defined for enum values. Because this reaper runs during FastAPI lifespan startup, the API will fail to boot as soon as it hits a database with any repositories table; cast the column to text or compare against enum literals without LOWER().
Useful? React with 👍 / 👎.
| try: | ||
| from src.dependencies import get_progress_store | ||
|
|
||
| event = await get_progress_store().latest(repo_id) |
There was a problem hiding this comment.
When Redis is enabled and the previous process dies mid-index, the Redis stream can still have parsing/embedding as its latest event while startup reaps the repository row to FAILED. Since this path returns the persisted progress before consulting the database, /api/repos/{id}/progress keeps reporting the stale in-progress state until timeout instead of the recoverable failure; clear/publish progress during reaping or reconcile the DB status before trusting the stream.
Useful? React with 👍 / 👎.
1 participant
Two bugs from the audit whose failure mode was silence, not an error.
1. Progress was write-only
IndexingServicewrote progress into a per-instance dict. The SSE endpoint builds its ownIndexingServiceper request, so it never saw those writes and always fell through to the database branch — which hardcodescurrent_step="Unknown"and a percentage of 0 or 100. The per-file progress the indexer computes could not reach any client.A per-process dict wouldn't fix it either: indexing runs in a background task and, with more than one worker, in a different process from the reader.
core/progress.pyadds aProgressStoreover a capped Redis stream per repository (progress:{repo_id}, MAXLEN 500) with a process-wide in-memory fallback. A stream rather than a key because the SSE endpoint wants history — a client connecting late can replay what it missed viaXRANGEinstead of seeing only the current value.Proof it actually fixes the reported bug — a fresh service instance with an empty dict, which is exactly what the SSE route constructs:
Publishing never raises. Progress is telemetry; a Redis blip must not fail a healthy index. There's a test using a Redis double that raises on every call, asserting the event still lands in memory.
2. A killed index was unrecoverable
Only
FAILEDandCOMPLETEDare terminal, and every self-healing path (repos.py,seed_demo.py) keys onFAILED— so a container killed duringCLONING/PARSING/EMBEDDINGleft the row stuck forever with no way to retry through the API. In demo mode that bricked the deployment.reap_stuck_indexingruns at startup, where anything still in a transient state provably has no live indexer, because the process that owned it is gone.Two deliberate choices, both tested:
PENDINGis left alone. A repository queued but not yet started is not stuck, and failing it would break the normal import path.indexing_erroris preserved. A real failure reason ("rate limited by provider") beats the generic interruption note.Also: the SSE route was unbounded
It looped forever with no client check and no ceiling, so a repository stuck mid-index held an open response and its database session indefinitely — once per browser tab. Now it checks
request.is_disconnected(), emits only on change instead of re-sending an identical payload every second, and stops after 15 minutes with a reconnect hint.Why Redis Streams and not Kafka
This is the durability primitive the tech-fit research recommended over Kafka, and the reasoning holds: Kafka's
max.poll.interval.msdefaults to 5 minutes whileindex_repositoryis a single minutes-long call, so a slow index triggers a rebalance and indexes the repo twice. Streams give at-least-once delivery andXAUTOCLAIM-based recovery on theredisservice already indocker-compose.yml— no new broker.Verification
ruff check src tests.envmoved aside;OPENAI_API_KEY,AZURE_OPENAI_API_KEY,ANTHROPIC_API_KEY,REDIS_URLall unsetThe Redis paths are exercised with fakeredis, which implements streams, consumer groups and
XAUTOCLAIM— so unlike the Neo4j PR, this code is genuinely executed, not mocked.🤖 Generated with Claude Code