Skip to content

fix(indexer): keep pool actor alive for queued batches - #511

Merged
hedhoud merged 2 commits into
refactor/hexagonalfrom
fix/indexer-pool-lifecycle
Jun 19, 2026
Merged

fix(indexer): keep pool actor alive for queued batches#511
hedhoud merged 2 commits into
refactor/hexagonalfrom
fix/indexer-pool-lifecycle

Conversation

@hedhoud

@hedhoud hedhoud commented Jun 18, 2026

Copy link
Copy Markdown
Collaborator

Context

Large indexing benchmarks can submit more work than the indexer can process immediately. In that case, queued tasks must remain attached to a live worker actor until their turn starts.

Problem

The indexer pool actor could be cleaned up by Ray while queued work still existed. Those tasks stayed visible as QUEUED, but nothing was left to consume them. A full dataset run could then stop after the first window of files.

Concurrent startup also let multiple tasks initialize the catalog store at the same time, which put unnecessary pressure on Postgres.

Expected behavior

The indexer pool remains alive for queued batches, respects the configured worker concurrency, and initializes its catalog connection once under concurrent load.

Validation

  • uv run pytest tests/unit/services/workers/test_indexer_pool.py -q
  • uv run pytest tests/unit/services/workers -q
  • uv run ruff format --check openrag/services/workers/indexer_pool.py tests/unit/services/workers/test_indexer_pool.py
  • uv run ruff check openrag/services/workers/indexer_pool.py tests/unit/services/workers/test_indexer_pool.py

Summary by CodeRabbit

  • Improvements
    • Improved indexing pool initialization to be safe under concurrent startup, ensuring the catalog is initialized only once.
    • Updated worker pool creation to respect configured concurrency settings and run the actor as a detached Ray worker for more reliable background operation.
  • Tests
    • Added unit coverage for single-flight catalog initialization under concurrent calls.
    • Added unit coverage verifying detached actor creation and correct concurrency configuration.

@coderabbitai

coderabbitai Bot commented Jun 18, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 3ccc511d-2609-416b-b3ee-e273956fd4b5

📥 Commits

Reviewing files that changed from the base of the PR and between b301be8 and d872f74.

📒 Files selected for processing (1)
  • openrag/services/workers/indexer_pool.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • openrag/services/workers/indexer_pool.py

📝 Walkthrough

Walkthrough

IndexerPool._ensure_catalog() is updated to use double-checked locking via a new asyncio.Lock, preventing concurrent catalog initialization. build_indexer_pool() now loads runtime config to set max_concurrency from cfg.ray.max_tasks_per_worker and adds lifetime="detached" to the Ray actor options. Two new unit tests cover both behaviors.

Changes

IndexerPool Concurrency and Actor Lifecycle

Layer / File(s) Summary
Double-checked locking for catalog initialization
openrag/services/workers/indexer_pool.py, tests/unit/services/workers/test_indexer_pool.py
_catalog_init_lock (asyncio.Lock) is added to IndexerPool; _ensure_catalog() checks _catalog_initialized before and after acquiring the lock. Concurrent async test verifies initialize() is called exactly once across parallel invocations.
Detached actor with configured max_concurrency
openrag/services/workers/indexer_pool.py, tests/unit/services/workers/test_indexer_pool.py
build_indexer_pool() loads config and passes max_concurrency=cfg.ray.max_tasks_per_worker and lifetime="detached" to IndexerPool.options(). Unit test stubs config and options().remote() to assert both values are forwarded correctly.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐇 A lock keeps the catalog safe from the fray,
Two coroutines racing — only one gets to stay!
The actor detaches, concurrent and free,
max_tasks_per_worker sets how busy we'll be.
One init to rule them, the rest hop away! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix(indexer): keep pool actor alive for queued batches' directly describes the primary change: preventing the indexer pool actor from being garbage collected while batches remain queued.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/indexer-pool-lifecycle

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Collapse the duplicated max_concurrency assignment into a single
expression. The bare cfg.ray.max_tasks_per_worker dropped the lower
bound, which Ray rejects (max_concurrency must be >= 1) when the
config is set to 0 or negative. The Pydantic model already guarantees
an int, so the int()/getattr() guards were redundant; keep only the
max(1, ...) floor.

@Ahmath-Gadji Ahmath-Gadji left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This solves the issue we had with batch indexing.

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

Labels

fix Fix issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants