Skip to content

indexer: process-pool symbol/edge extraction (#108) - #119

Merged
IceRhymers merged 1 commit into
integration/indexer-performancefrom
feat/108-process-pool-extraction
Jul 26, 2026
Merged

indexer: process-pool symbol/edge extraction (#108)#119
IceRhymers merged 1 commit into
integration/indexer-performancefrom
feat/108-process-pool-extraction

Conversation

@IceRhymers

Copy link
Copy Markdown
Owner

Summary

Symbol/edge extraction (indexer.symbols.extract_file) is CPU-bound tree-sitter work that holds the GIL through parse(), so a repo's own worker thread cannot parallelize it. This adds a shared, spawn-based ProcessPoolExecutor (indexer/extract_pool.py, new) built once per run and threaded through run()_index_one_index_one_inner_index_one_branch, so a single dominant repo's files parse on every available core instead of one thread's worth.

Executed from .omc/plans/issue-108/approved-plan.md (critic-APPROVEd after 3 review cycles, revalidated 2026-07-25 against 5f290c6). This PR targets integration/indexer-performance, not master** — #108 is delivery item 6/7 of epic #110, closed by that arc's umbrella PR #111, not by this child PR. **Refs #108, not Closes #108`.**

Design (each point driven by a measured or reproduced failure mode — see the plan and indexer/extract_pool.py's module docstring)

  • spawn, never fork/forkserver. Workers spawn lazily on first submit(), so build-order alone does not make fork safe against the repo worker's own thread pool, live SQLAlchemy engine, and (since indexer: concurrent embedding requests, order-preserving #107) a per-embed ThreadPoolExecutor.
  • A bare, terminable multiprocessing.Process preflight probe, run before any ProcessPoolExecutor exists. An executor-based probe (executor.submit() + future.result(timeout=...)) cannot be cancelled once running and hangs the interpreter at exit — reproduced. Since resources/job.yml has max_concurrent_runs: 1 and no timeout_seconds, that would block every queued run indefinitely instead of degrading to "a WARNING and a run at today's speed".
  • A generation-tagged supervisor for BrokenProcessPool. Rebuilds once per generation (CAS under a lock — verified race-free with a two-thread test), bounded by MAX_POOL_REBUILDS = 3, then latches to in-process extraction for the rest of the run.
    • Honest blast radius: a shared pool's break fails every repo worker holding a future at that moment (up to effective_workers branches — ≤4 by default, ≤2 with semantic on), not "one branch" as AC3's wording literally reads. Each failed branch self-heals on its next run (never got a stamp). Stated this way in the runbook and AGENTS.md rather than parroting AC3.
  • No prefetch thread. Since indexer: single-pass in-memory tarball ingestion (drop extract-to-disk) #106 the production source (iter_tar_source_files) is a single-pass, single-open TarFile; stream() pulls it strictly from the calling thread, with bounded look-ahead achieved by pulling more inside the consumer's own next(), never concurrently. A second thread advancing that generator would silently corrupt output, not raise.
  • Zero-byte diff to the four semantics-watched paths (indexer/symbols.py, parse.py, languages.py, ingest.py) and to app/db/models.py — extraction output is unchanged by construction (the pool calls the same unmodified extract_file), so no INDEX_SEMANTICS_VERSION bump. indexer/extract_pool.py is deliberately not added to SEMANTICS_PATHS: it changes where extraction runs, never what is extracted.
  • New extract_processes config field (RepoConfig, 1..8, default derived from affinity/cgroup-aware CPU count capped at 8 — same ceiling as index_concurrency and embedding_concurrency). extract_processes: 1 is the kill switch: no pool is ever built, stream() degrades to today's in-process expression. Config-only rollback, no redeploy, no migration, no re-index.

AC evidence

AC2 (parity; no semantics bump) — MET.

$ git diff --name-only origin/integration/indexer-performance...HEAD
config.yaml
docs/perf/issue-108-measurements.md
docs/runbooks/indexing-parallelism.md
indexer/AGENTS.md
indexer/extract_pool.py
indexer/job.py
indexer/repo_config.py
scripts/measure_extraction_pool.py
tests/unit/test_extract_pool.py
tests/unit/test_job.py
tests/unit/test_repo_config.py

None of indexer/symbols.py, indexer/parse.py, indexer/languages.py, indexer/ingest.py, or app/db/models.py appear — this is AC2's real evidence, per the plan. tests/unit/test_extract_pool.py's T1 additionally proves whole-list equality between the pooled path and extract_file called directly, through a real spawn pool (not a fake executor), across all 7 languages.

AC3 (poisoned file fails one branch; run continues) — MET, reported honestly (see blast-radius note above). Covered by a two-concurrent-thread test that drives an actual race on the supervisor's rebuild-once-per-generation logic, plus a test_job.py case proving one branch is classified failed while the next branch of the same repo indexes normally.

AC1 (parse+extract scales ≥3x on 4+ cores) — NOT MET as literally worded on the measured corpus. Reporting this as a stop-and-report finding per the plan's own binding instruction (§8.4/R12), not shipping it quietly.

scripts/measure_extraction_pool.py, driving the real ExtractionPool.stream() over a real tarball via the real production source (iter_tar_source_files), on this repo's own working tree at the branch point (4547 indexable files / 54.5 MB, 2749 qualifying / 45.7 MB):

path extract_s total_s speedup
serial 6.563 7.355 1.00x
pool x2 3.423 4.215 1.74x
pool x4 1.913 2.705 2.72x
pool x8 1.422 2.214 3.32x

Reproduced across 3 separate invocations (2.72–2.75x at 4 processes, stable, not noise). Full writeup, environment, and the serial-fraction split in docs/perf/issue-108-measurements.md.

Why: since #106 the file source is a single serial gzip-decompress+decode+filter pass on the repo-worker thread (iter_tar_source_files), which this pool cannot parallelize — on this corpus that pass is ~11% of the serial total. By Amdahl's law that caps combined speedup at ~3.03x even at ideal 4x extraction-only speedup; the measured extraction-only number (6.563/1.913 = 3.43x, ~86% parallel efficiency) is real and good — the shortfall is structural (a serial floor #106 introduced), not a defect in this pool's own parallel efficiency. At 8 processes the combined number does clear 3x (3.22–3.32x).

The design itself (fork-safety, BrokenProcessPool blast-radius containment, the terminable preflight probe, bounded look-ahead, order preservation) is sound and independently valuable — it more-than-doubles extraction throughput at 4 processes and more than triples it at 8, a real win for any dominant-repo run. But AC1's specific "≥3x on 4+ cores" bar is not met at the "4" endpoint on this corpus, and per the plan's binding instruction this must be escalated rather than tuned around. Flagging for explicit operator sign-off rather than blocking; happy to hold this PR if the answer is "fix AC1 first" instead of "ship with the caveat and let #109/a follow-up target ingest parallelism next."

AC4 (runbook §3 superseded) — MET. New ### Extraction process pool (#108) subsection; all four repo-wide 0.95x occurrences updated (config.yaml:12, docs/runbooks/indexing-parallelism.md, indexer/repo_config.py:267, tests/unit/test_job.py); limits table gains the extract_processes row and the ~121 MB/worker RSS term.

Gates

  • make lint (ruff check + ruff format --check + mypy app indexer webui) — clean.
  • make test (pytest -m "unit or observability") — 1313 passed, exactly 1 failure: tests/unit/test_semantics_version_tripwire.py::test_semantics_change_bumps_the_index_semantics_version. This is inherited from the base branch, not caused by this change — indexer/ingest.py (added by indexer: single-pass in-memory tarball ingestion (drop extract-to-disk) #106) is an added path relative to origin/master with no paired INDEX_SEMANTICS_VERSION bump in that range. Verified by running the same test against the unmodified origin/integration/indexer-performance commit in a separate worktree: identical failure, byte-for-byte same assertion message. Per the plan (§4.1.1/§9.2/R10), the correct response is exactly this — prove it pre-exists, report both runs, change nothing (no version bump, no SEMANTICS_PATHS edit, no CI change — that's ci: unit job's shallow checkout defeats the semantics-version tripwire #113's scope).
  • make test-integration (pytest -m "integration or e2e", against a local codesearch-pg pgvector/pg16 container) — the tests that matter for indexer: process-pool symbol/edge extraction #108 (test_store.py, test_store_batching.py, test_store_delta.py, test_job_reconcile.py, test_chunk_batching.py, test_job_ingest_delta.py) do not need Lakebase-only operators: 56/56 passed. The rest of the suite (test_semantic_rrf.py, test_migrations.py 0004+, test_reconcile.py, test_store_chunk_writer.py, test_webui_semantic.py, plus a couple of test_commit_search.py/test_mcp_server.py cases) fails/errors against this box's vanilla pgvector because it lacks the real Lakebase lakebase_tokenizer/lakebase_ann/lakebase_bm25 beta operators — a known, pre-existing environment gap (not introduced here; CI's ci-lakebase.yml gate covers this properly and is known unprovisioned repo-wide, so it no-ops on every PR as it always has).

Review

Ran an independent code-reviewer pass and a dedicated security-reviewer pass (both fresh-context, against the diff before this PR's final fixup commit). Neither raised a CRITICAL or HIGH finding. Fixed before opening this PR:

  • The preflight probe caught only OSError, so the exact RuntimeError an unguarded python_wheel_task __main__ raises (multiprocessing.spawn._check_not_importing_main) would have escaped its intended WARNING path — broadened to except Exception per the plan's own §4.9 wording, and the ambiguous "exited with code 0" message for a missing-sentinel failure is now a distinct message.
  • A runbook log-line example named the wrong logger (indexer.job instead of the real indexer.extract_pool).
  • _MAX_BATCH_FILES was documented as a per-batch bound but only ever enforced as a look-ahead-window bound — a corpus of many near-zero-byte qualifying files could pickle one 32,000-file batch onto a single idle worker. Now enforced per-batch too, with a regression test.
  • Added a test pinning that the shared pool is built before the run's first database read (test_pool_is_built_before_the_first_db_read) — this ordering is what bounds a re-entrant python_wheel_task child (under an unguarded __main__) from becoming a second corpus writer, per the plan's R2/§10.3; the test exists so a future refactor can't silently move the pool build later without a red test.
  • A stray resource_tracker: leaked semaphore stderr line from the probe's kill path is now called out in the runbook as expected noise, not a separate problem.

Deferred, not blocking, both explicitly discussed and accepted rather than silently dropped:

  • Credential-scrub initializer= for spawned workers (defense-in-depth; workers today inherit the full parent environment including PGPASSWORD/LAKEBASE_* despite never using it) — real hardening, but new design surface beyond the approved plan's D-items; flagging as a candidate follow-up rather than adding unreviewed scope this late.
  • Attributing a BrokenProcessPool break to the poisoned file's path in the error message — would materially help 2am triage, but is the first place this change would start putting repo-derived paths into a log/exception message; flagging for a deliberate follow-up decision rather than folding it in under review pressure.
  • One LOW finding (the generation/executor reference is captured once per stream() call rather than re-checked at each submit()) was verified to fail safely (still raises BrokenProcessPool, never corrupts output) and only marginally widens the already-documented "up to effective_workers branches" blast radius — left as-is.

Other honest notes

Test plan

  • make lint clean
  • make test — 1313 passed, 1 pre-existing inherited failure (proven on base commit, see Gates)
  • make test-integration — 56/56 for the indexer: process-pool symbol/edge extraction #108-relevant suites; remainder blocked on Lakebase-only extensions unavailable in this local Postgres, as expected
  • Independent code review + security review, both COMMENT-level (no blockers), findings applied
  • AC1 measurement re-run against the real tarball source and reported honestly, including the sub-3x-at-4-processes finding

Symbol/edge extraction (indexer.symbols.extract_file) is CPU-bound
tree-sitter work that holds the GIL, so a repo's own worker thread cannot
parallelize it. This adds a shared, spawn-based ProcessPoolExecutor
(indexer/extract_pool.py) built once per run and threaded through
run() -> _index_one -> _index_one_inner -> _index_one_branch, so a single
dominant repo's files parse on every available core instead of one
thread's worth.

Design highlights, each driven by a measured or reproduced failure mode:
- spawn, never fork/forkserver (workers spawn lazily on first submit(),
  so build-order does not make fork safe against the repo worker's own
  thread pool, live DB engine, and httpx client).
- A bare, terminable multiprocessing.Process preflight probe, run before
  any ProcessPoolExecutor exists -- an executor-based probe cannot be
  cancelled on timeout and hangs the interpreter at exit, which would
  block every queued run indefinitely under this job's
  max_concurrent_runs: 1 with no timeout_seconds.
- A generation-tagged supervisor for BrokenProcessPool: rebuild once per
  generation (CAS under a lock), bounded by MAX_POOL_REBUILDS, then latch
  to in-process extraction for the rest of the run. A shared pool's break
  fails every repo worker holding a future at that moment (up to
  effective_workers branches, each self-healing on its next run), not
  just one -- stated honestly rather than as "one branch".
- No prefetch thread: the production source (iter_tar_source_files) is a
  single-pass TarFile, and stream() pulls it strictly from the calling
  thread with bounded look-ahead instead.

indexer/symbols.py, parse.py, languages.py, and ingest.py carry a
zero-byte diff -- extraction output is unchanged by construction, so no
INDEX_SEMANTICS_VERSION bump. New extract_processes config field
(RepoConfig, 1..8, default derived from affinity/cgroup CPU count capped
at 8; 1 is the kill switch, mirroring embedding_concurrency's rollback
shape).

Refs #108
@IceRhymers
IceRhymers merged commit b6379db into integration/indexer-performance Jul 26, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant