feat(daemon): close two automagic-invariants convergence gaps - #3286
Conversation
Problem: unlike every other mechanical, non-judgment maintenance operation (FTS merge, WAL checkpoint, embedding orphan reconcile — all of which already have a daemon-owned periodic equivalent), `polylogue ops maintenance blob-gc` had no automatic path at all. Reclaiming an unreferenced, aged-out content-addressed blob is a pure mechanical decision (refcount==0 and past the generation-age gate, storage/blob_gc.py) with no judgment content, so leaving it manual-invocation-only was a plain, un-tracked gap against this repo's "if Polylogue can maintain a condition fully automatically, it generally should" doctrine (polylogue-gd6v/4jsk). What changed: new polylogue/daemon/blob_gc_periodic.py runs one bounded batch (max 200, matching embedding-orphan-reconcile's shape) every 900s through the daemon write coordinator, mirroring periodic_embedding_orphan_reconcile_check's structure exactly (same transient-lock retry handling, same catch_up_complete gating). The existing safety invariants in run_blob_gc_report (referenced-blob check, generation-age gate, bounded batch, planning outside the destructive lock) are unchanged -- this only adds a caller. Verification: devtools test tests/unit/daemon/test_blob_gc_periodic.py (4 passed, including a DaemonWriteCoordinator production-route proof); devtools verify --quick (pass). Co-Authored-By: Claude <noreply@anthropic.com>
Problem: this command's own docstring already admitted the anti-pattern the automagic-invariants ruling (polylogue-gd6v/4jsk, 2026-07-19) names and forbids: "daemon convergence reconciles these automatically... this command remains the break-glass inspect/apply path." The ruling is explicit that a redundant manual surface must be deleted once its automatic equivalent exists, not demoted to break-glass status -- periodic_embedding_orphan_reconcile_check (polylogue/daemon/ embedding_backlog.py) has run this exact reconciliation on a 900s cadence since it shipped, making the CLI's --yes apply path pure standing debt. What changed: embedding-orphan-reconcile is now permanently read-only -- removed --yes, the RebuildLease/offline-exclusive apply branch, and the dead dry_run-false rendering branch (report.dry_run is always True by construction now). Removed the apply-path tests (cli_apply_removes_rows, cli_apply_is_bounded_by_default, cli_apply_requires_exclusive_offline_lease, cli_apply_refuses_stale_index_schema) and added one proving --yes is rejected as an unknown option. Verification: devtools test tests/unit/cli/test_archive_maintenance_cli.py -k embedding_orphan (4 passed); devtools verify --quick (pass). 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: 31 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 (1)
📝 WalkthroughWalkthroughThe change makes embedding orphan reconciliation strictly read-only, adds daemon-managed periodic blob garbage collection with coordinator execution and tests, and refreshes generated topology metadata and status counts. ChangesEmbedding orphan diagnostics
Periodic blob garbage collection
Topology metadata refresh
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Daemon
participant DaemonWriteCoordinator
participant BlobGC
Daemon->>DaemonWriteCoordinator: Schedule maintenance.blob_gc
DaemonWriteCoordinator->>BlobGC: Run bounded garbage collection
BlobGC-->>DaemonWriteCoordinator: Return reclaimed blobs and bytes
DaemonWriteCoordinator-->>Daemon: Return result or transient lock error
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/cli/test_archive_maintenance_cli.py`:
- Around line 1301-1308: Update the embedding-orphan-reconcile CLI test to avoid
asserting Click’s unstable “no such option” wording. Keep the rejected-option
assertion by requiring exit code 2, verify result.exception is a Click
UsageError (or equivalent expected exception), and retain the check that “--yes”
appears in the output.
In `@tests/unit/daemon/test_blob_gc_periodic.py`:
- Around line 62-69: Update the test helper _backdate to use the repository’s
frozen_clock fixture when setting the blob mtime, deriving an old timestamp from
the frozen instant rather than hard-coding epoch zero. Ensure the age-gate test
remains deterministically older than MIN_AGE_S while using frozen_clock for all
timestamp-sensitive behavior.
- Around line 112-117: Update the test’s run function to call the synchronous
DaemonWriteCoordinator.run_sync method without await, and invoke it directly
instead of wrapping it with asyncio.run. Remove the now-unused asyncio import
while preserving the existing arguments and result assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 927ba287-0c60-405c-bbf9-76174055b57d
📒 Files selected for processing (7)
docs/plans/topology-target.yamldocs/topology-status.mdpolylogue/cli/commands/maintenance/_embeddings.pypolylogue/daemon/blob_gc_periodic.pypolylogue/daemon/cli.pytests/unit/cli/test_archive_maintenance_cli.pytests/unit/daemon/test_blob_gc_periodic.py
| def _backdate(blob_store: BlobStore, blob_hash: str) -> None: | ||
| """Backdate a blob's mtime well past MIN_AGE_S (60s) so it is GC-eligible. | ||
|
|
||
| A fixed epoch (not a `time.time()`-relative offset) avoids any host-clock | ||
| read in the test itself -- GC eligibility only needs "old enough", not a | ||
| real elapsed duration. | ||
| """ | ||
| os.utime(blob_store.blob_path(blob_hash), (0, 0)) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use frozen_clock for the age-gate test.
This test is timestamp-sensitive but hard-codes an epoch instead of using the repository’s required frozen_clock fixture. Derive the old mtime from the frozen instant, or freeze the clock around the GC call.
As per coding guidelines, timestamp-sensitive tests must use frozen_clock.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/daemon/test_blob_gc_periodic.py` around lines 62 - 69, Update the
test helper _backdate to use the repository’s frozen_clock fixture when setting
the blob mtime, deriving an old timestamp from the frozen instant rather than
hard-coding epoch zero. Ensure the age-gate test remains deterministically older
than MIN_AGE_S while using frozen_clock for all timestamp-sensitive behavior.
Source: Coding guidelines
| coordinator = DaemonWriteCoordinator() | ||
|
|
||
| async def run() -> object: | ||
| return await coordinator.run_sync("maintenance.blob_gc", run_blob_gc_once, db_path, blob_dir) | ||
|
|
||
| result = asyncio.run(run()) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not await the synchronous coordinator API.
DaemonWriteCoordinator.run_sync is a regular synchronous method (see polylogue/daemon/write_coordinator.py, Lines 403-413). Awaiting its returned BlobGCResult raises TypeError, so this test fails before its assertions.
Suggested fix
- async def run() -> object:
- return await coordinator.run_sync("maintenance.blob_gc", run_blob_gc_once, db_path, blob_dir)
-
- result = asyncio.run(run())
+ result = coordinator.run_sync("maintenance.blob_gc", run_blob_gc_once, db_path, blob_dir)Remove the now-unused asyncio import as part of the fix.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| coordinator = DaemonWriteCoordinator() | |
| async def run() -> object: | |
| return await coordinator.run_sync("maintenance.blob_gc", run_blob_gc_once, db_path, blob_dir) | |
| result = asyncio.run(run()) | |
| coordinator = DaemonWriteCoordinator() | |
| result = coordinator.run_sync("maintenance.blob_gc", run_blob_gc_once, db_path, blob_dir) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/daemon/test_blob_gc_periodic.py` around lines 112 - 117, Update
the test’s run function to call the synchronous DaemonWriteCoordinator.run_sync
method without await, and invoke it directly instead of wrapping it with
asyncio.run. Remove the now-unused asyncio import while preserving the existing
arguments and result assertions.
CodeRabbit review on #3286: "no such option" matches Click's current NoSuchOption text but isn't a stable public contract. Assert exit_code == 2 (Click's CliRunner converts a rejected option into SystemExit(2) before invoke() returns, so the original UsageError itself is not what result.exception carries) plus the option name in the output instead. Co-Authored-By: Claude <noreply@anthropic.com>
|
Addressed the Click-wording finding (now asserts The other two don't hold up: "Do not await the synchronous coordinator API" — this is incorrect for the class actually used here. "Use frozen_clock for the age-gate test" — not applicable: |
Problem Auditing ops maintenance/doctor manual repair surfaces against the automagic-invariants doctrine (polylogue-cfvvt) against PR #3286's own precedent found an inconsistency: that PR added daemon/blob_gc_periodic.py's periodic_blob_gc_check (reclaiming eligible blobs automatically every 900s in bounded batches through the write coordinator) and, in the same change, removed embedding-orphan-reconcile's --yes apply path because its own periodic reconciler made it redundant. blob-gc's --yes survived that PR by omission even though the identical reasoning applies: the manual mutate path is now doing nothing periodic_blob_gc_check doesn't already do. Solution - polylogue/cli/commands/maintenance/_blob_gc.py: removed --yes and the apply branch from blob_gc_command; the command is now always a dry-run preview (mutates always False), matching embedding-orphan-reconcile's read-only shape. gc-history (audit trail read) is untouched. storage/blob_gc.py (run_blob_gc_report, dry_run param) is untouched -- the daemon's periodic caller still uses it with dry_run=False. - Updated the command's help text in maintenance/__init__.py and replaced test_blob_gc_cli_yes_deletes_and_records_generation with test_blob_gc_cli_has_no_mutate_flag, mirroring test_embedding_orphan_reconcile_cli_has_no_mutate_flag's exit-code-2/--yes-rejected assertion pattern. - polylogue/storage/message_type_backfill.py: added a docstring paragraph documenting why this doctor-repair target is KEEP (finite one-time legacy-row remediation, no recurring daemon condition since new-ingest rows are classified correctly since PR #836/#944) as part of the same audit. The audit's other findings (blob-reference-prune-orphans/ replace-from-source and embeddings-rescue: KEEP, no daemon path exists and both require genuine judgment or are one-time migrations; doctor's 5 destructive cleanup targets: KEEP, destructive=True by design with no daemon caller; doctor's session_insights repair target: proven redundant by the same shape but scoped out to a dedicated follow-up, polylogue-ygfwa, given its wider surface coupling) required no code changes and are recorded in polylogue-cfvvt's notes. Verification - devtools test tests/unit/cli/test_archive_maintenance_cli.py -k blob_gc -- 3 passed - devtools test tests/unit/daemon/test_blob_gc_periodic.py -- 4 passed - devtools render all --check -- clean, no "out of sync" surfaces - devtools verify --quick -- exit 0, all 19 steps green - tests/unit/storage/test_blob_gc.py's 7 failures are pre-existing (clock_guard time.time() violations in code this change never touches) and unrelated to this diff Ref polylogue-cfvvt Co-Authored-By: Claude <noreply@anthropic.com>
## Summary Audits four candidate manual `ops maintenance`/`ops doctor` repair surfaces (polylogue-cfvvt) against the automagic-invariants doctrine and deletes the one confirmed-redundant mutate path found: `ops maintenance blob-gc --yes`. ## Problem PR #3286 added `daemon/blob_gc_periodic.py`'s `periodic_blob_gc_check` (reclaims eligible, aged-out, unreferenced blobs automatically every 900s in bounded batches of 200 through the write coordinator) and, in the same change, removed `embedding-orphan-reconcile`'s `--yes` apply path because its own periodic reconciler made manual apply redundant. `blob-gc`'s `--yes` survived that PR by omission even though the identical reasoning applies: the manual mutate path does nothing `periodic_blob_gc_check` doesn't already do on its own. ## Solution - `polylogue/cli/commands/maintenance/_blob_gc.py`: removed `--yes` and the apply branch from `blob_gc_command`; the command is now always a dry-run preview (`mutates` always `False`), matching `embedding-orphan-reconcile`'s read-only shape. `gc-history` (audit-trail read) is untouched. `storage/blob_gc.py` (`run_blob_gc_report`, `dry_run` param) is untouched — the daemon's periodic caller still uses it with `dry_run=False`. - Updated the command's catalog help text in `maintenance/__init__.py`. - Replaced `test_blob_gc_cli_yes_deletes_and_records_generation` with `test_blob_gc_cli_has_no_mutate_flag`, mirroring `test_embedding_orphan_reconcile_cli_has_no_mutate_flag`'s exit-code-2/`--yes`-rejected assertion pattern. - `polylogue/storage/message_type_backfill.py`: added a docstring paragraph documenting why this doctor-repair target is a legitimate KEEP (finite one-time legacy-row remediation; no recurring daemon condition since new-ingest rows are classified correctly since PR #836/#944), recorded as part of the same audit. The audit's other findings required no code changes and are recorded in `polylogue-cfvvt`'s notes: - `blob-reference-prune-orphans`/`blob-reference-replace-from-source`: KEEP — no daemon path exists for `blob_refs` debt at all, and both commands require genuine judgment per missing blob. - `embeddings-rescue`: KEEP — already documented as an explicit one-time break-glass migration, structurally undetectable by daemon convergence. - Doctor's 5 destructive cleanup targets (`orphaned_messages`, `empty_sessions`, `orphaned_attachments`, `orphaned_blobs`, `superseded_raw_snapshots`): KEEP — `destructive=True` by catalog design, zero daemon caller, genuinely consent-gated. - Doctor's `session_insights` repair target: proven to have the same redundancy shape (covered by both a per-ingest `ConvergenceStage` and a dedicated periodic `convergence_debt` retry loop), but scoped out to a dedicated follow-up (`polylogue-ygfwa`) given its wider surface coupling (`REPAIR_HANDLERS`, maintenance planner/replay, archive-debt status reporting) versus `blob-gc`'s single CLI file. ## Verification - `devtools test tests/unit/cli/test_archive_maintenance_cli.py -k blob_gc` — 3 passed - `devtools test tests/unit/daemon/test_blob_gc_periodic.py` — 4 passed - `devtools render all --check` — clean, no "out of sync" surfaces - `devtools verify --quick` — exit 0, all 19 steps green - `tests/unit/storage/test_blob_gc.py`'s 7 failures are pre-existing (`clock_guard` `time.time()` violations in code this change never touches) and unrelated to this diff Ref polylogue-cfvvt
1 participant
Summary
Closes two verified gaps found while auditing the daemon's "automagic invariants" doctrine against actual behavior: blob-GC had no periodic daemon path at all, and
embedding-orphan-reconcile's manual apply path was redundant with (and admittedly demoted-not-deleted next to) its own automatic daemon equivalent.Problem
An audit comparing this repo's stated doctrine ("if Polylogue can maintain a condition fully automatically, it generally should... the redundant manual surface is DELETED, not demoted to escape-hatch status", polylogue-gd6v/4jsk) against actual runtime behavior found:
polylogue ops maintenance blob-gc(reclaiming unreferenced, aged-out content-addressed blobs — a purely mechanical refcount/age decision) had no daemon-periodic equivalent at all, unlike every comparable mechanical maintenance operation (FTS merge, WAL checkpoint, embedding orphan reconcile).embedding-orphan-reconcile's--yesapply path is redundant:periodic_embedding_orphan_reconcile_checkhas run the exact same reconciliation automatically every 900s since it shipped, and the CLI docstring already said as much — "this command remains the break-glass inspect/apply path" is the precise anti-pattern the 2026-07-19 ruling names and forbids.Two other candidate gaps from the same audit turned out not to be real gaps on closer inspection, and are intentionally NOT touched by this PR:
missing_bytes_reacquireraw-authority obligations are already re-classified every 30s by_converge_raw_authority_frontier(part of the existing raw-materialization convergence loop) — no gap.judgment_automation_enabled(assertion-candidate auto-judgment, default off) is an unrelated subsystem from raw-authority frontier conflicts, and its default is an explicit write-authority security boundary, not something to flip as a side effect of this audit.Solution
polylogue/daemon/blob_gc_periodic.py:periodic_blob_gc_checkruns one bounded batch (max 200 blobs) every 900s through the daemon write coordinator, structurally mirroringperiodic_embedding_orphan_reconcile_check(same transient-lock retry handling, samecatch_up_completegating). No changes torun_blob_gc_report's existing safety invariants (referenced-blob check, generation-age gate, bounded batch, planning outside the destructive lock) — this only adds a caller.embedding-orphan-reconcileis now permanently read-only: removed--yes, theRebuildLease/offline-exclusive apply branch, and the now-deaddry_run=Falserendering branch.Verification
devtools test tests/unit/daemon/test_blob_gc_periodic.py— 4 passed, including aDaemonWriteCoordinatorproduction-route proof (bypassingrun_syncmakes the test fail).devtools test tests/unit/cli/test_archive_maintenance_cli.py -k embedding_orphan— 4 passed, including a new test proving--yesis now rejected as an unknown option.devtools verify --quick— pass.Summary by CodeRabbit
New Features
Bug Fixes
--yesoption and manual deletion capability from the reconciliation command.Documentation