fix: sweep orphaned attachment rows when their last ref is dropped - #3514
Conversation
Problem: forensics on the live archive (polylogue-w06b) found 1,858 `attachments` rows with zero `attachment_refs` rows -- 1,783 of them `acquisition_status='acquired'` with 440MB of real fetched blob bytes, permanently unreachable from every session/message read path (`get_attachments` and friends INNER JOIN attachment_refs). Of the archive's 1,933 'acquired' attachments, only 150 (7.8%) were actually queryable. Root cause (classification: (a), a straightforward missing write/cleanup, not a backfill-needs-reconstruction situation): `_write_attachments` in `archive_tiers/write.py` already recomputes `ref_count` for attachments whose ref was dropped by a full-replace re-ingest (message removed, duplicate-native-id exclusion, etc.), but -- unlike the identical cleanup already performed by `prune_attachments` and `delete_session_sql` -- it never deleted the row once `ref_count` hit 0. The row survived forever, still reporting `acquisition_status='acquired'`. Solution: - `_write_attachments` now deletes `attachments` rows whose `ref_count` drops to 0 among the ids it just refreshed, mirroring the two other writers that already do this cleanup. - `AttachmentAcquisitionDebtReport` (`storage/blob_integrity.py`) gains `acquired_reachable_count`/`acquired_unreachable_count` (+ a sample), computed via a `NOT EXISTS (... attachment_refs ...)` check, so `attachment-acquisition-debt` no longer reports `acquisition_status='acquired'` as if it meant "queryable" -- AC3 of polylogue-w06b (reconciling the coverage framing used by polylogue-7r6u/pfdf). Non-goal: backfilling the 1,783 existing orphaned rows in the live archive. Their original owning message was already dropped by the ingest that orphaned them, so reconstructing the link (if possible at all) requires re-parsing original source exports, not a mechanical fix -- this PR does not touch the live archive. A follow-up bead should track that backfill/GC decision for the existing rows; this PR only stops the leak going forward. Verification: - `devtools test tests/unit/storage/test_attachment_acquisition.py tests/unit/storage/test_blob_integrity.py tests/unit/storage/test_archive_tiers_write.py tests/unit/storage/test_attachment_first_class_ids.py` -- 111 passed. - Anti-vacuity: reverted the new `DELETE FROM attachments WHERE ref_count <= 0 ...` statement and reran `test_orphaned_attachment_ref_is_swept_not_left_unreachable` -- it fails (`assert <sqlite3.Row ...> is None`), reproducing the exact live-archive bug before restoring the fix. - `devtools verify --quick` -- exit 0 (format, lint, mypy --strict, render all --check). - `devtools test tests/unit/cli/test_archive_maintenance_cli.py -k attachment_acquisition` -- 2 passed. Two unrelated pre-existing failures in the same file (test_rebuild_index_full_source_resumes_one_candidate_until_terminal_promotion, test_rebuild_index_byte_budget_defers_then_reaches_terminal_ready_candidate) were confirmed to fail identically with this change reverted -- pre-existing, not caused by this PR. Ref polylogue-w06b Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 12 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 |
Continuing the devtools lab probe bead-pr-reconciliation sweep started in 820b396. polylogue-w06b: PR #3514 satisfies AC1 (root cause found) and AC3 (coverage framing reconciled via acquired_reachable_count/ acquired_unreachable_count) but only partially satisfies AC2 — going-forward writes now GC orphans at write time, but the PR explicitly defers backfilling the 1,783 existing orphaned rows already in the live archive as a "follow-up bead needed" non-goal, and no such follow-up bead has been filed yet. Left open pending that remediation. Also reviewed: the 13-item (11 currently-valid) "open parent, all children closed" review queue from the beads-state-report and dozens of additional bead-pr-reconciliation candidates (6j9c/9kjtc reprice-pass residual, hjpx raw-authority closure gated on child yla8, rxdo.9.x/60i5/stc/9l5.7 judgment epics, cijx.1/cijx.4/oqib repo-identity slices, t46.8.2/t46.8.3/t46.9 MCP migration, pzxm/o56w/623q rebuild-perf epics, cost-accounting cluster gt1z/shnc/yhgc/iuyr/qwgi, and ~70 more). All were found either already accurately tracked by a same-day "stale-sweep"/"RECONCILE"/"adjudication" note, or the referencing PR explicitly states the bead is filed/deferred/ left-open rather than resolved (a "Ref #N"-style incidental mention, not a resolution claim) — no bd write needed for those. Ref polylogue-93xe Co-Authored-By: Claude <noreply@anthropic.com>
…rse (#3553) ## Summary Adds a targeted repair for the 1,858 `attachments` rows polylogue-w06b's forensics found with zero `attachment_refs` rows, and closes a data-loss hazard in the existing production repair path that would otherwise delete them unrecoverably. ## Problem PR #3514 fixed the write-path gap that produced new orphans going forward (`_write_attachments` now sweeps a ref-less row at write time), but explicitly deferred the 1,858 orphans already in the live archive: `attachments` carries no `session_id`/`message_id` column, and `attachment_id` is a content-identity hash of the attachment's own metadata, independent of which session produced it. The only durable evidence that can reconstruct "which message owned this orphan" is the original raw bytes still held in `source.db`'s `raw_sessions`. Separately, the existing production repair (`repair_orphaned_attachments`, `polylogue/storage/repair.py`) makes this worse: its `dry_run=False` branch unconditionally `DELETE`s every ref-less `attachments` row, including the 1,783 `acquired` ones (440MB of real fetched bytes), without ever attempting to recover them first. Run as-is against the live archive, this repair would permanently destroy exactly the rows this bead asks to recover. ## Solution - `polylogue/storage/attachment_relink.py`: `plan_orphaned_attachment_relink` (read-only) re-parses `source.db` `raw_sessions` rows via `ingest_record` -- the same decode/parse/materialize entry point the live ingest worker uses -- and checks every attachment it reproduces against the pending orphan set. A match is only accepted `eligible` when the recomputed `attachment_id` equals the orphan's AND the resolved `message_id` is a real row in the current `messages` table; every other outcome (no raw reproduces the identity, or a raw reproduces it but the owning message is gone) is reported `ineligible` with an exact reason -- it never guesses. `relink_orphaned_attachments` (`dry_run=True` default) writes the recovered `attachment_refs` rows, mirroring the exact `INSERT` `_write_attachments` uses. Follows the plan/execute split already used by `raw_retention.plan_stale_supersession_reissue`. - `polylogue/storage/repair.py`: `repair_orphaned_attachments` now calls a best-effort relink pass before its destructive `DELETE`, so the existing production maintenance sweep can no longer silently discard a recoverable orphan. Never blocks the cleanup: a missing `source.db` or any re-parse failure just means 0 relinked, not a repair failure. ## Non-goals - Does not run against `/realm/db/polylogue` (production archive) -- this worktree only tests against fixtures. Whether any of the live 1,858 orphans are actually recoverable this way depends on whether their raw sessions are still retained in `source.db`; that census is a follow-up operator action, not something provable from a fixture-only worktree. - Does not change `repair_orphaned_attachments`'s preview/dry-run path to attempt relink (a full raw re-parse scan is heavier than a routine count-only preview should do); only the actual execute path attempts it. ## Verification - `devtools test tests/unit/storage/test_attachment_relink.py` -> 4 passed, including a real end-to-end test (genuine raw JSON bytes in a real `source.db` + blob store, parsed via the actual `ingest_record` entry point, no mocking) that reproduces the exact live-archive orphan symptom (`ref_count` 0, `acquisition_status='acquired'`) and verifies both the plan classification and that the production read path (`get_attachments`) can see the attachment after relink. A separate test proves an orphan with no matching raw is reported unrecoverable, not guessed. - `devtools test tests/unit/storage/test_attachment_relink.py tests/unit/storage/test_repair.py tests/unit/storage/test_attachment_acquisition.py tests/unit/storage/test_archive_tiers_write.py tests/integration/test_health.py -k "not raw_materialization"` -> 128 passed. - 11 pre-existing `raw_materialization` test failures in `test_repair.py` observed and confirmed unrelated: same 11 tests, same failure shape, documented as pre-existing in PR #3514's own verification section (unrelated subsystem: raw-authority census/replay, not attachments). - `devtools verify --quick` -> exit 0 (format, lint, mypy --strict, render all --check, topology projection regenerated for the new module). Also ran again by the pre-push hook -> exit 0. - Anti-vacuity: reverting the `INSERT OR REPLACE INTO attachment_refs` write in `relink_orphaned_attachments` makes `test_orphaned_attachment_is_reachable_via_production_read_path_after_relink` fail, since `get_attachments` INNER JOINs `attachment_refs`. Production callers exercised: `get_attachments` (`storage/sqlite/queries/attachment_records.py`, the read path every attachment surface goes through) and `repair_orphaned_attachments` (the production maintenance entry point, `storage/repair.py`). Ref polylogue-w06b Co-authored-by: Claude <noreply@anthropic.com>
…shots - status.py: PR #3649's new Polylogue facade method reconcile_codex_spawn_edges was undiscovered in the archive-facade route catalog (registration-trap class, per this repo's own convention). - Snapshot refresh: SOURCE_SCHEMA_VERSION 22->24 (today's byte-dup supersession + verdict-cache migrations); aistudio-drive schema drift warning gone (tu1f's gemini catalog identity backfill, PR #3648, genuinely resolved it). - test_prepare.py: PR #3514 (pre-existing, not today) swept ref-less attachments rows outright instead of leaving ref_count=0 rows behind -- test still expected the old retained-row behavior. Co-Authored-By: Claude <noreply@anthropic.com>
Summary
Fixes an index.db-tier write-path gap where
_write_attachmentscould leave a ref-lessattachmentsrow behind forever, unreachable from every session/message read path, and adds a reachability split to the existing attachment-acquisition-debt report so that gap can't hide behindacquisition_status='acquired'again.Problem
Read-only forensics on the live archive (polylogue-w06b, 2026-08-01) found 1,858
attachmentsrows inindex.dbwith zeroattachment_refsrows:1,783 of those were
acquisition_status='acquired'(440MB of real, successfully-fetched blob content) but permanently unreachable from every session-scoped read path —get_attachments/get_attachments_batch(storage/sqlite/queries/attachment_records.py) bothINNER JOIN attachment_refs, so a ref-less row can never be returned to any surface (MCP, CLIread --view, transcript view). Of the archive's 1,933 attachments markedacquired, only 150 (7.8%) were actually queryable.Root cause classification: (a) — a straightforward missing cleanup step at write time, not a case needing reconstruction/backfill logic.
_write_attachments(polylogue/storage/sqlite/archive_tiers/write.py) already recomputesref_countfor attachments whose ref was dropped on a full-replace re-ingest (the owning message disappeared this pass — dropped, renumbered, or excluded as a duplicate native id), via:But unlike the identical cleanup already performed by
prune_attachments(queries/attachment_mutations.py) anddelete_session_sql(queries/sessions_writes.py) — both of which follow their own ref-count refresh withDELETE FROM attachments WHERE ref_count <= 0—_write_attachmentsnever had that DELETE. The row survives indefinitely, still reportingacquisition_status='acquired'.Solution
_write_attachmentsnow runs the sameDELETE FROM attachments WHERE ref_count <= 0 AND attachment_id IN (...)sweep the other two writers already perform, scoped to the sameaffected_attachment_idsset the ref_count UPDATE just touched.AttachmentAcquisitionDebtReport(polylogue/storage/blob_integrity.py, backing theattachment-acquisition-debtCLI command) gainsacquired_reachable_count/acquired_unreachable_count(+ a sample of unreachable ids), computed via aNOT EXISTS (SELECT 1 FROM attachment_refs ...)check againstattachments.acquisition_status='acquired'. This reconciles the "attachment coverage" framing used by polylogue-7r6u/pfdf (AC3 of polylogue-w06b):acquired_countalone overstated real coverage, since it never distinguished "bytes fetched" from "bytes anyone can actually read back".ops maintenance attachment-acquisition-debt) now breaks out reachable/unreachable under theAcquired:line and lists a sample of unreachable ids when present.Non-goals
/realm/db/polyloguein any way. A follow-up bead should track the backfill/GC decision for the existing rows (delete them outright vs. attempt re-parse-based relinking) — flagging this for the coordinator to file, since this lane's worktree must not write.beads/.attachment_refs' schema (e.g. adding anON DELETE CASCADEfromattachmentsback to some caller) — the gap was in application-level write discipline, not the schema.Verification
devtools test tests/unit/storage/test_attachment_acquisition.py tests/unit/storage/test_blob_integrity.py tests/unit/storage/test_archive_tiers_write.py tests/unit/storage/test_attachment_first_class_ids.py→ 111 passed.DELETE FROM attachments WHERE ref_count <= 0 ...statement and rerantest_orphaned_attachment_ref_is_swept_not_left_unreachable— it fails (assert <sqlite3.Row object at ...> is None), reproducing the exact live-archive bug shape before restoring the fix. Production caller exercised:get_attachments(storage/sqlite/queries/attachment_records.py), the read path every session/message attachment surface goes through.devtools test tests/unit/cli/test_archive_maintenance_cli.py -k attachment_acquisition→ 2 passed.devtools verify --quick→ exit 0 (format, lint, mypy --strict, render all --check — regenerated docs stayed in sync, no drift).tests/unit/cli/test_archive_maintenance_cli.py(test_rebuild_index_full_source_resumes_one_candidate_until_terminal_promotion,test_rebuild_index_byte_budget_defers_then_reaches_terminal_ready_candidate) and intests/unit/storage/test_repair.py(11 raw-materialization tests) — confirmed to fail identically with this branch's changes reverted, so classified as pre-existing/unrelated, not caused by this PR.AC matrix (polylogue-w06b)
_write_attachments, evidenced above.acquired_reachable_count/acquired_unreachable_countonAttachmentAcquisitionDebtReport+ CLI output.Ref polylogue-w06b