perf(maintenance): warm the offline rebuild-index route's prefetch cache - #3477
Conversation
Problem: RebuildIndexRequest.prefetch_cache (the off-writer-hold census pre-parse seam from PR #3168) had exactly one caller that ever populated it -- the daemon's own bulk-rebuild loop (daemon/bulk_rebuild.py). The offline `polylogue ops maintenance rebuild-index` CLI, and the daemon's own POST /api/maintenance/rebuild-index HTTP route, both construct a RebuildIndexRequest without ever threading a cache, so census always paid the full unwarmed parse cost on those routes. The operator's real 4h22m CLI rebuild measured spill_load (serial re-deserialization inline on the writer thread) at 2,830s -- 70% of the run's parse_s. Solution: relocated the parse-stage warmer (formerly polylogue.daemon.parse_prefetch.DaemonParseStage, consulted by exactly one caller) to substrate as polylogue.sources.census_parse_stage.CensusParseStage -- it had no daemon-specific behavior, only a daemon-specific import path. polylogue.daemon.parse_prefetch now re-exports the same objects unchanged so every existing daemon call site and test keeps working byte-identically (verified: tests/unit/daemon/test_parse_prefetch.py and 5 other daemon/ sources/storage test files, 110 tests, all green). maintenance/rebuild_index.py's _rebuild_index_from_source_owned now calls a new _warm_offline_prefetch_cache helper whenever a caller leaves request.prefetch_cache at its None default, warming exactly this pass's own selected_raw_ids in a bounded thread pool before replay. This fixes BOTH the offline CLI route and the daemon's own HTTP rebuild-index route in one place (both call the same engine function), rather than needing two separate call-site patches -- and composes with PR #3468's prepare_session_rows threading without touching it. daemon/bulk_rebuild.py's own external off-writer-hold warm is untouched (its caller-supplied cache is never overridden). New test (test_rebuild_index_from_source_sync_warms_prefetch_cache_when_ caller_omits_one) drives the real rebuild_index_from_source_sync entry point with a request that omits prefetch_cache and asserts CensusParseStage.warm_raw_ids was actually reached with this pass's raw ids -- verified to fail when the fix is reverted (temporarily disabled the internal warm call, confirmed the new assertion fails with "never warmed a prefetch cache", then restored it). Measurement: a synthetic 400-raw/60KB corpus (tests.infra. revision_backfill_benchmark) shows census stage time collapsing from ~0.8s to ~0.1-0.15s with warming enabled -- but spill_load stayed 0.0s in both runs, because this corpus is far too small to overflow the ~256MiB-2GiB decoded-session hot cache that gates whether spill_load pays sqlite pickle.loads at all. This harness measurably under-represents the real whale-corpus path the operator's receipt came from (per the task brief: ~10% untimed synthetic work vs ~33% real) -- the census-time collapse is real and directly measured; the spill_load collapse the real 87K-raw run would show is not independently reproduced here and should not be claimed as measured. Follow-up filed: polylogue-mznm (the daemon's own whale-scale raw- materialization pass has the identical prefetch_cache=None gap against its sibling trickle pass -- but needs a NEW warm step, not just threading an existing variable, so scoped out of this PR); polylogue-q1at (proposal to make this defect class -- a capability adopted by one caller of a shared engine while siblings keep the disabled default -- detectable by tooling). Verification: devtools verify --quick (ruff format/check, mypy --strict, render all --check, topology, layering, all ok); devtools test across the 12 affected daemon/sources/storage/maintenance/cli test files (110 passed); anti-vacuity check on the new test (fails when the fix is disabled). Co-Authored-By: Claude <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
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. Comment |
…o-resolved beads/generated surfaces)
1 participant
Summary
The offline
polylogue ops maintenance rebuild-indexCLI and the daemon's ownPOST /api/maintenance/rebuild-indexHTTP route both build aRebuildIndexRequestwithout ever populatingprefetch_cache, so they silently pay the full unwarmed census parse cost that the daemon's bulk-rebuild loop (daemon/bulk_rebuild.py, #3168) has always avoided by warming ahead of the writer hold. This PR closes that gap for both routes in one place.Problem
RebuildIndexRequest.prefetch_cache(the off-writer-hold census pre-parse seam introduced by #3168) had exactly one caller in the whole codebase that ever populated it:daemon/bulk_rebuild.py's automagic bulk-rebuild routing. Every other caller of the shared rebuild engine — the offline CLIrebuild-indexcommand, and the daemon's own HTTP rebuild-index route — left it at itsNonedefault.The operator's real 4h22m CLI rebuild run measured
spill_load(serial re-deserialization of already-parsed sessions, inline on the writer thread) at 2,830s — 70% of the run'sparse_s(4,032s). The daemon's bulk-rebuild route would not have paid this the same way.Solution
polylogue.daemon.parse_prefetch.DaemonParseStage(daemon-only, one caller) to substrate aspolylogue.sources.census_parse_stage.CensusParseStage— the mechanism has no daemon-specific behavior, only a daemon-specific import path.polylogue.daemon.parse_prefetchnow re-exports the same objects unchanged (verified: 6 existing daemon/sources/storage test files still pass, 110 tests total, all unmodified in behavior).maintenance/rebuild_index.py's_rebuild_index_from_source_ownednow calls a new_warm_offline_prefetch_cachehelper whenever a caller leavesrequest.prefetch_cacheat itsNonedefault, warming exactly this pass's ownselected_raw_idsin a bounded thread pool before replay. This fixes both the offline CLI route and the daemon's own HTTP route in one place (both call the same engine function), instead of needing two separate call-site patches. It composes with perf(storage): wire prepare_session_rows off the writer thread #3468'sprepare_session_rowsthreading without touching it, and never overrides a cache a caller already supplied (daemon/bulk_rebuild.py's own external off-writer-hold warm is untouched).docs/plans/layering-surface-baseline.json(5 staledaemon/parse_prefetch.pyimport entries replaced by 1 newcensus_parse_stageentry) and regenerateddocs/plans/topology-target.yamlfor the new module.Alternatives rejected
cli/commands/maintenance/_rebuild_index.py): rejected because the CLI would need to re-derive the exact page/raw_id selection the engine's own resumable-transaction logic computes internally, risking a second, drifting implementation of that selection policy — the same defect class this PR is trying to close, not reproduce.DaemonParseStageindaemon/parse_prefetch.pyand importing it frommaintenance/rebuild_index.py: rejected becausemaintenanceis documented as the substrate engine CLI/daemon/HTTP adapt over (rebuild_index.py's own module docstring), so a substrate module reaching into a surface package would invert that relationship even though today'slayering.yamlruleset happens not to forbid it explicitly.Verification
devtools verify --quick(ruff format/check, mypy --strict,render all --check, topology, layering) — all green.devtools testacross the 12 affected files (tests/unit/daemon/test_parse_prefetch.py,test_config_resolution_regression.py,test_daemon_bulk_rebuild_responsiveness.py,test_live_watcher_parse_stage_equivalence.py,test_prefetch_cache_thread_safety.py,test_rebuild_paging_content_order.py,test_raw_materialization_parse_stage_equivalence.py,test_bulk_rebuild.py,test_rebuild_parse_apply_split.py,test_rebuild_index_ownership.py,test_rebuild_index_selection.py,test_rebuild_index_bulk_build.py,test_archive_maintenance_cli.py) — 110 passed.test_rebuild_index_from_source_sync_warms_prefetch_cache_when_caller_omits_onedrives the realrebuild_index_from_source_syncentry point (the exact function both the CLI and HTTP route call) with a request that omitsprefetch_cache, and assertsCensusParseStage.warm_raw_idswas actually reached with this pass's raw ids. Anti-vacuity verified manually: temporarily disabled the internal warm call, confirmed the assertion fails with "offline rebuild_index_from_source_sync never warmed a prefetch cache", then restored the fix.tests.infra.revision_backfill_benchmark):censusstage time collapses from ~0.8s to ~0.1-0.15s with warming enabled.spill_loadstayed at 0.0s in both runs — this synthetic corpus is far too small to overflow the ~256MiB-2GiB decoded-session hot cache that gates whetherspill_loadever pays apickle.loadsreload at all, so the real 87K-raw whale-corpusspill_loadcollapse from the operator's receipt is not independently reproduced here and is not claimed as measured. The task brief's own caveat applies: this harness under-represents the real path (~10% untimed synthetic work vs ~33% real).Follow-ups
polylogue-mznm: the daemon's own whale-scale raw-materialization escalation pass (_run_raw_materialization_whale_pass_once) has the identicalprefetch_cache=Nonegap against its sibling trickle pass — scoped out of this PR because it needs a new warm-before-writer-hold step (no equivalent variable exists in scope yet), not just threading an existing one.polylogue-q1at: proposal to make this defect class — a capability adopted by one caller of a shared engine while sibling callers silently keep the disabled default — detectable by tooling, calibrated against this session's corpus of already-fixed instances.