feat(storage): relink orphaned attachments recoverable from raw re-parse - #3553
Conversation
Problem polylogue-w06b forensics found 1,858 attachments rows with zero attachment_refs rows -- unreachable from every session-scoped read path. PR #3514 already fixed the write-path gap that produced new orphans (_write_attachments now sweeps a ref-less row at write time), but it explicitly deferred the 1,858 existing orphans already in the live archive, since attachments carries no session_id/message_id column and attachment_id is a content-identity hash 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. The existing production repair path (repair_orphaned_attachments, polylogue/storage/repair.py) makes this worse: its dry_run=False branch unconditionally DELETEs 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 this PR can prove from a fixture-only worktree. - Does not change repair_orphaned_attachments' 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). - 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>
|
Warning Review limit reached
Next review available in: 41 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 (4)
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 |
Summary
Adds a targeted repair for the 1,858
attachmentsrows polylogue-w06b's forensics found with zeroattachment_refsrows, 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_attachmentsnow sweeps a ref-less row at write time), but explicitly deferred the 1,858 orphans already in the live archive:attachmentscarries nosession_id/message_idcolumn, andattachment_idis 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 insource.db'sraw_sessions.Separately, the existing production repair (
repair_orphaned_attachments,polylogue/storage/repair.py) makes this worse: itsdry_run=Falsebranch unconditionallyDELETEs every ref-lessattachmentsrow, including the 1,783acquiredones (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-parsessource.dbraw_sessionsrows viaingest_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 acceptedeligiblewhen the recomputedattachment_idequals the orphan's AND the resolvedmessage_idis a real row in the currentmessagestable; every other outcome (no raw reproduces the identity, or a raw reproduces it but the owning message is gone) is reportedineligiblewith an exact reason -- it never guesses.relink_orphaned_attachments(dry_run=Truedefault) writes the recoveredattachment_refsrows, mirroring the exactINSERT_write_attachmentsuses. Follows the plan/execute split already used byraw_retention.plan_stale_supersession_reissue.polylogue/storage/repair.py:repair_orphaned_attachmentsnow calls a best-effort relink pass before its destructiveDELETE, so the existing production maintenance sweep can no longer silently discard a recoverable orphan. Never blocks the cleanup: a missingsource.dbor any re-parse failure just means 0 relinked, not a repair failure.Non-goals
/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 insource.db; that census is a follow-up operator action, not something provable from a fixture-only worktree.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 realsource.db+ blob store, parsed via the actualingest_recordentry point, no mocking) that reproduces the exact live-archive orphan symptom (ref_count0,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.raw_materializationtest failures intest_repair.pyobserved and confirmed unrelated: same 11 tests, same failure shape, documented as pre-existing in PR fix: sweep orphaned attachment rows when their last ref is dropped #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.INSERT OR REPLACE INTO attachment_refswrite inrelink_orphaned_attachmentsmakestest_orphaned_attachment_is_reachable_via_production_read_path_after_relinkfail, sinceget_attachmentsINNER JOINsattachment_refs. Production callers exercised:get_attachments(storage/sqlite/queries/attachment_records.py, the read path every attachment surface goes through) andrepair_orphaned_attachments(the production maintenance entry point,storage/repair.py).Ref polylogue-w06b