fix(daemon): close rebuild-index safety gaps from CodeRabbit review - #3318
Conversation
Problem: PR #3076 (daemon-coordinated online index rebuilds) shipped with 8 CodeRabbit findings never addressed, tracked as debt in polylogue-ogn1 because the merge task was scoped to a conflict-free rebase, not review. What changed, verified against current source (not blindly reapplied): 1. HTTP handler must reject max_blob_mb without raw_ids/only_missing before promoting -- ALREADY FIXED prior to merge (validate_rebuild_index_request already enforced this in the merged commit 1c2a07f; confirmed via `git show 1c2a07f:polylogue/maintenance/rebuild_index.py`). No change. 2/5. write_coordinator.py's bridge held a fixed ~30s run_sync timeout while the CLI/HTTP rebuild contract allows up to 600s. Added DaemonWriteThreadBridge.run_sync_with_timeout (a per-call override, scoped to this need rather than a general timeout-override framework); run_sync now delegates to it with the bridge's own default. The rebuild-index HTTP route calls it with a 600s budget matching the CLI's own urlopen(..., timeout=600). 3. CLI --daemon-url default read POLYLOGUE_DAEMON_URL directly via os.environ.get, bypassing the 5-layer config precedence chain every other daemon-URL consumer goes through. Now resolves via load_polylogue_config().daemon_url, matching status.py's own _default_daemon_url pattern. 4. HTTP handler fell back to executing the rebuild directly when server.write_bridge was missing, instead of failing closed. A real DaemonAPIHTTPServer always installs write_bridge in __init__, so this was unreachable in production, but a route that CAN execute an authority-promoting archive write outside the sole-writer coordinator is a bypass of the daemon's single-writer invariant, not a safe fallback. Now returns 503 write_coordinator_unavailable instead. 6. missing_index_raw_ids returned [] whenever index.db did not exist yet, making --only-missing rebuild nothing on a fresh archive or right after `ops reset --index` -- exactly the case it exists to handle. Now falls back to the full source set (every row is missing by definition). 7/9. Partial-selection-cannot-promote and empty-source-count-0 guards -- ALREADY enforced inside the shared rebuild_index_from_source service itself (validate_rebuild_index_request called first thing; raw_count==0 short-circuit before any replay), not merely the CLI. Confirmed present in the original merged commit. No change. 8. rebuild_index.py (substrate) imported the CLI's private _archive_readiness_status, inverting this repo's documented surfaces-may-not-import-substrate rule. Extracted the readiness-surface computation (archive_readiness_status + its _archive_readiness_counts/ _action_readiness_counts/_archive_status_surfaces helpers) into polylogue/storage/archive_readiness.py; status.py now delegates to the same shared implementation instead of owning the only copy. 10. test_rebuild_index_route_uses_the_bridge_run_sync_writer_path replaced _handle_rebuild_index wholesale with its own body calling bridge.run_sync -- it only proved the test's own stand-in worked, never the real production handler. Rewritten to drive the real _do_post_impl dispatch and the real _handle_rebuild_index implementation end to end. Solution: polylogue/daemon/write_coordinator.py (run_sync_with_timeout), polylogue/daemon/http.py (fail-closed + 600s timeout on rebuild-index), polylogue/cli/commands/maintenance/_rebuild_index.py (config-resolved daemon URL), polylogue/maintenance/rebuild_index.py (missing_index_raw_ids fix), polylogue/storage/archive_readiness.py (readiness extraction), polylogue/cli/commands/status.py (delegates instead of owning). New/updated tests prove each fix: a rejected unsafe fallback (test_maintenance_endpoints .py), a real per-call timeout override (test_write_coordinator.py), a fresh-index --only-missing selection (test_rebuild_index_selection.py), and a real-dispatch rebuild-index route proof (test_http_write_coordination.py). Verification: - mypy polylogue (strict): Success: no issues found in 1082 source files - ruff check . / ruff format --check .: All checks passed / all formatted - devtools verify --quick: exit_code 0 (includes degrade-loudly, now allowlisted for the two extracted typed-signal except-handlers) - devtools render all --check: no "out of sync" lines, exit 0 - devtools test across the affected area (status/rebuild_index/daemon http/ write_coordinator/maintenance CLI): 264 passed, 1 pre-existing unrelated failure (test_archive_facade_route_catalog_covers_public_async_facade, confirmed failing identically on origin/master before this change via `git stash`) Ref polylogue-ogn1 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: 35 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 (12)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
1 participant
Summary
Addresses the 8 CodeRabbit findings on PR #3076 (daemon-coordinated online
index rebuilds) that were left unaddressed at merge time, tracked as
polylogue-ogn1. Each finding was independently re-verified against current
source before any fix was applied — several turned out to already be
resolved by the time this bead was filed.
Problem
polylogue-ogn1 notes: "CodeRabbit's review on the PR (commit 667ad8e,
before the final rebase) flagged 8 actionable findings that were not
addressed before merge because the merging task was scoped specifically to
a conflict-free rebase onto origin/master, not a full code-review pass."
Disposition matrix (finding -> status, evidence)
max_blob_mbwithoutraw_ids/only_missingbefore promotingvalidate_rebuild_index_requestalready enforced this in the originally merged commit1c2a07f9b(git show 1c2a07f9b:polylogue/maintenance/rebuild_index.py), called first thing insiderebuild_index_from_sourceand again explicitly in the HTTP handler before dispatch. No change needed.run_synctimeout vs. the 600s CLI/HTTP rebuild contractDaemonWriteThreadBridge.run_sync_with_timeout(per-call override);run_syncnow delegates to it with the bridge's own default so every other caller is unaffected. The rebuild-index HTTP route calls it with_REBUILD_INDEX_WRITE_TIMEOUT_S = 600.0, matching the CLI's ownurlopen(..., timeout=600).--daemon-urldefault readPOLYLOGUE_DAEMON_URLdirectly, bypassing the 5-layer config chain_rebuild_index.pynow has its own_default_daemon_url()resolving viaload_polylogue_config().daemon_url, matchingstatus.py's established pattern.write_bridgewas missing, instead of failing closedDaemonAPIHTTPServeralways installswrite_bridgein__init__(confirmed by reading the constructor), so this branch was unreachable in production — but a route that can execute an authority-promoting write outside the sole-writer coordinator is a bypass of the daemon's single-writer invariant, not a safe fallback. Now returns503 write_coordinator_unavailable.missing_index_raw_idsreturned[]wheneverindex.dbdidn't exist yetall_index_rebuild_raw_ids) when the index tier is absent — a fresh/lost index has nothing indexed, so every source row is missing by definition. Previously--only-missingsilently rebuilt nothing on a fresh archive or right afterops reset --index.validate_rebuild_index_requestis called as the first statement inside the sharedrebuild_index_from_source, so every caller (CLI, HTTP) goes through it.rebuild_index.py(substrate) imported the CLI's private_archive_readiness_status, inverting the repo's layering rulepolylogue/storage/archive_readiness.py(archive_readiness_status+ its_archive_readiness_counts/_action_readiness_counts/_archive_status_surfaceshelpers);status.pynow delegates to the same shared implementation instead of owning the only copy.raw_count == 0), unlike the CLI's own early exitraw_count == 0short-circuit lives insiderebuild_index_from_sourceitself, before any replay, executed by every caller.test_rebuild_index_route_uses_the_bridge_run_sync_writer_pathreplaced_handle_rebuild_indexwholesale, proving only its own stand-intest_rebuild_index_route_uses_the_bridge_run_sync_with_timeout_writer_path) to drive the real_do_post_impldispatch and the real_handle_rebuild_indeximplementation end to end, with only the typed rebuild service itself stubbed.Solution
polylogue/daemon/write_coordinator.py— newrun_sync_with_timeout.polylogue/daemon/http.py— fail-closed on missing write bridge; 600stimeout constant for the rebuild-index route.
polylogue/cli/commands/maintenance/_rebuild_index.py— config-resolveddaemon URL default.
polylogue/maintenance/rebuild_index.py—missing_index_raw_idsfresh-indexfix; import switched to the extracted shared readiness module.
polylogue/storage/archive_readiness.py— newarchive_readiness_status(+ helpers), the layering fix for refactor: rebrand as polylogue and move config/state to XDG #8.
polylogue/cli/commands/status.py— delegates to the shared module insteadof owning the readiness computation.
tests/unit/daemon/test_maintenance_endpoints.py(fail-closed proof),tests/unit/daemon/test_write_coordinator.py(real timeout-override proof),tests/unit/maintenance/test_rebuild_index_selection.py(new, fresh-indexselection + shared-service validation),
tests/unit/daemon/test_http_write_coordination.py(real-dispatch rewrite),
tests/unit/cli/commands/test_status.py(importfixup for the moved functions).
Verification
mypy polylogue(strict):Success: no issues found in 1082 source filesruff check ./ruff format --check .: all checks passed / all formatteddevtools verify --quick:exit_code: 0(includesdegrade-loudly, nowallowlisted with rationale for the two extracted typed-signal except-handlers
in
docs/plans/degrade-loudly-allowlist.yaml)devtools render all --check: noout of synclines, exit 0devtools testacross the affected area (status/rebuild_index/daemon http/write_coordinator/maintenance CLI, 8 files): 264 passed, 1 pre-existing
unrelated failure (
test_archive_facade_route_catalog_covers_public_async_facade,confirmed failing identically on
origin/masterviagit stashbeforethis change — a facade/hook-event route catalog completeness check
unrelated to rebuild-index)
Ref polylogue-ogn1
Co-Authored-By: Claude noreply@anthropic.com