fix(maintenance): remove blob-gc's redundant --yes mutate path - #3575
Merged
Conversation
|
Warning Review limit reached
Next review available in: 27 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 (5)
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 |
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>
Ref polylogue-cfvvt Co-Authored-By: Claude <noreply@anthropic.com>
Sinity
force-pushed
the
worktree-agent-ade1f1fa4d4bc9c51
branch
from
August 2, 2026 17:05
ce31813 to
302ea73
Compare
Sinity
added a commit
that referenced
this pull request
Aug 2, 2026
…ndant Problem: polylogue-cfvvt's automagic-invariants audit found that `doctor --repair --target session_insights` looks covered by two daemon automatic mechanisms (the per-ingest insights ConvergenceStage and the periodic convergence_debt retry loop), the same redundancy shape used to justify removing blob-gc's --yes mutate path (#3575). It deferred a final call to polylogue-ygfwa pending a closer look, since session_insights repair is threaded through more surface than a single-file deletion. Solution: traced both automatic mechanisms (make_insights_stage in daemon/convergence_stages.py and the periodic drain in daemon/cli.py) and confirmed they only ever call rebuild_session_insights_sync (per-session profile/work_events/phases). Neither calls refresh_session_insight_aggregates_sync, the archive-wide refresh of thread materialization, tag rollups, and provider-day aggregates that repair_session_insights runs when aggregate debt is nonzero -- verified via grep that repair.py is the only caller of that function in the tree. repair_session_insights is also reused directly by maintenance/rebuild_index.py's terminal stage to materialize insights for a freshly built inactive generation before promotion, a scenario the daemon (which only touches the live/active generation) cannot reach. This is a real gap, not audit-summary redundancy: KEEP-WITH- REASON, not DELETE. Documented the specific mechanism gap inline on repair_session_insights so a future redundancy pass doesn't have to re-derive it. Verification: devtools verify --quick (exit 0, all 19 checks pass); docstring-only change, no behavior touched. Ref polylogue-ygfwa
Sinity
added a commit
that referenced
this pull request
Aug 2, 2026
## Summary Documents, inline on `repair_session_insights`, the concrete mechanism gap that makes `doctor --repair --target session_insights` genuinely not redundant with the daemon's automatic convergence paths. No behavior change. ## Problem polylogue-cfvvt's automagic-invariants audit found `doctor --repair --target session_insights` structurally similar to the redundancy shape that justified removing blob-gc's `--yes` mutate path (#3575): two automatic mechanisms already exist (the per-ingest insights `ConvergenceStage`, and a periodic `convergence_debt` retry loop). It deliberately deferred a final delete-or-keep call to polylogue-ygfwa rather than rushing it, since session_insights repair is threaded through more surface (`REPAIR_HANDLERS`, the maintenance planner/replay executor, archive-debt status reporting) than a single CLI file. ## Solution Traced both automatic mechanisms end to end: - `make_insights_stage` (`polylogue/daemon/convergence_stages.py`) - the periodic drain loop (`_periodic_convergence_check` → `_retry_convergence_debt_once` → `_drain_convergence_debt_once` in `polylogue/daemon/cli.py`), which just re-drives `make_default_convergence_stages` Both only ever call `rebuild_session_insights_sync` (per-session profile/work_events/phases). Neither calls `refresh_session_insight_aggregates_sync` — the archive-wide refresh of thread materialization (`threads`/`thread_sessions`), tag rollups (`session_tag_rollups`), and provider-day aggregates. `repair_session_insights` is the *only* caller of `refresh_session_insight_aggregates_sync` in the tree (confirmed by grep across `daemon/` and the rest of the codebase); it invokes that refresh whenever `_session_insight_aggregate_debt_count` (`missing_thread_materialization_count`, `stale_thread_count`, `orphan_thread_count`, `stale_tag_rollup_count`, `stale_day_summary_count`) is nonzero. So a `SESSION_INSIGHT_MATERIALIZER_VERSION` bump (or anything else that stales thread/tag-rollup aggregates archive-wide) leaves that debt stuck forever unless the manual repair runs — the daemon has no automatic route to clear it. `repair_session_insights` is also reused directly (not via the doctor CLI) by `maintenance/rebuild_index.py`'s terminal stage, to materialize insights for a freshly built *inactive* generation before promotion — a scenario the daemon (which only ever touches the live/active generation) cannot reach at all. **Disposition: KEEP-WITH-REASON**, following the same evidence bar as the `rebuild-index` investigation (polylogue-rpuqn), which similarly found the daemon path was not actually equivalent at full scope and correctly kept the manual command. ## Verification `devtools verify --quick` — exit 0, all 19 checks pass (docstring-only change, static gates only; no behavior touched, no test selection needed). Ref polylogue-ygfwa
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Audits four candidate manual
ops maintenance/ops doctorrepair 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'speriodic_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, removedembedding-orphan-reconcile's--yesapply path because its own periodic reconciler made manual apply redundant.blob-gc's--yessurvived that PR by omission even though the identical reasoning applies: the manual mutate path does nothingperiodic_blob_gc_checkdoesn't already do on its own.Solution
polylogue/cli/commands/maintenance/_blob_gc.py: removed--yesand the apply branch fromblob_gc_command; the command is now always a dry-run preview (mutatesalwaysFalse), matchingembedding-orphan-reconcile's read-only shape.gc-history(audit-trail read) is untouched.storage/blob_gc.py(run_blob_gc_report,dry_runparam) is untouched — the daemon's periodic caller still uses it withdry_run=False.maintenance/__init__.py.test_blob_gc_cli_yes_deletes_and_records_generationwithtest_blob_gc_cli_has_no_mutate_flag, mirroringtest_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 fix(archive): classify runtime artifacts as non-prose #836/fix(reconciliation): restore baseline and audit closed-scope drift #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 forblob_refsdebt 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.orphaned_messages,empty_sessions,orphaned_attachments,orphaned_blobs,superseded_raw_snapshots): KEEP —destructive=Trueby catalog design, zero daemon caller, genuinely consent-gated.session_insightsrepair target: proven to have the same redundancy shape (covered by both a per-ingestConvergenceStageand a dedicated periodicconvergence_debtretry 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) versusblob-gc's single CLI file.Verification
devtools test tests/unit/cli/test_archive_maintenance_cli.py -k blob_gc— 3 passeddevtools test tests/unit/daemon/test_blob_gc_periodic.py— 4 passeddevtools render all --check— clean, no "out of sync" surfacesdevtools verify --quick— exit 0, all 19 steps greentests/unit/storage/test_blob_gc.py's 7 failures are pre-existing (clock_guardtime.time()violations in code this change never touches) and unrelated to this diffRef polylogue-cfvvt