fix(storage): stop requiring an impossible index-tier backup attestation - #3596
Conversation
Problem: two mutation actuators that protect a derived-tier (index.db) change behind a backup manifest -- agent_meta_sidecar_purge_apply.py (polylogue-ioz7) and attachment_reacquisition.py -- called validate_migration_backup_manifest(..., ArchiveTier.INDEX, ...), which unconditionally requires a cryptographic HMAC attestation for that tier. daemon/backup.py's _write_successful_verification_receipt only ever mints attestations for durable tiers (source, user), by design -- index.db and embeddings.db were never wired for one, matching the architecture's durable-vs-derived split. So no backup manifest, however fresh or complete, could ever satisfy the check: --apply against either actuator was permanently broken. Found running the operator-authorized ioz7 live purge against a same-day, all-5-tiers-present, verdict=success manifest. What changed: migration_runner.py's validation logic is split into a shared internal helper plus two entry points. validate_migration_backup_manifest keeps its existing behavior (attestation required) for durable-tier migrations (migrate_archive_tier, unchanged). The new validate_backup_manifest_covers_derived_tier skips the attestation check for non-durable tiers, while keeping every other guarantee: manifest/ receipt shape, tier inclusion, receipt/manifest byte match, and a byte-exact live fingerprint (sha256/size/user_version) recomputed from the current on-disk file. Both purge_apply.py and attachment_reacquisition.py now call the new function for their index.db check. Anti-vacuity: every existing test for both actuators monkeypatched the validator entirely, so none of them ever exercised a manifest produced by the real `polylogue ops backup --profile full_evidence --verify` path -- exactly the gap that hid this bug. Added test_apply_accepts_a_real_backup_manifest_from_ops_backup, which builds the fixture archive, runs the real backup_archive() entry point against it, and lets apply_agent_meta_sidecar_purge validate that real manifest with no monkeypatching. Verification: devtools test on both actuators' test files (14 passed) + the durable-tier siblings that still use validate_migration_backup_manifest unchanged (raw_authority, raw_append_chain_backfill_apply, raw_membership_writeback_apply, raw_live_source_reconciliation_apply, durable_migrations -- 55 passed, confirming the durable-tier path is untouched). mypy --strict on all 5 touched files -- clean. Ref polylogue-5kmn7, polylogue-ioz7
|
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 (2)
📝 WalkthroughWalkthroughBackup-manifest validation now uses a shared implementation with separate durable-tier and derived-tier modes. Index-tier callers use the derived-tier validator during preflight and apply. Tests add real-backup purge coverage and update validator patches. ChangesDerived-Tier Backup Validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PurgeOrReacquisition
participant DerivedTierValidator
participant BackupEvidence
participant IndexDB
PurgeOrReacquisition->>DerivedTierValidator: validate index-tier backup coverage
DerivedTierValidator->>BackupEvidence: verify manifest, receipt, artifact, and inventory
DerivedTierValidator->>IndexDB: compare live fingerprint
DerivedTierValidator-->>PurgeOrReacquisition: return validated backup path
PurgeOrReacquisition->>IndexDB: apply purge or reacquisition
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 1
🤖 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 `@polylogue/maintenance/agent_meta_sidecar_purge_apply.py`:
- Line 201: Both mutation flows revalidate the backup manifest too early; add a
post-lock fingerprint validation. In
polylogue/maintenance/agent_meta_sidecar_purge_apply.py:201, acquire the write
lock used by ArchiveStore.delete_sessions, then rerun
validate_backup_manifest_covers_derived_tier before deletion. In
polylogue/storage/attachment_reacquisition.py:557-563, execute BEGIN IMMEDIATE
before the second validation and preserve rollback handling when validation
fails. Add a concurrency regression that mutates index.db between precheck and
lock acquisition and verifies the apply operation rejects the stale backup.
🪄 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: 48b0a921-b5dc-4361-830a-b54dcf69ef60
📒 Files selected for processing (5)
polylogue/maintenance/agent_meta_sidecar_purge_apply.pypolylogue/storage/attachment_reacquisition.pypolylogue/storage/sqlite/migration_runner.pytests/unit/maintenance/test_agent_meta_sidecar_purge_apply.pytests/unit/storage/test_attachment_reacquisition.py
| try: | ||
| _checkpoint_live_tier(validate_conn) | ||
| validate_migration_backup_manifest(backup_manifest, ArchiveTier.INDEX, connection=validate_conn) | ||
| validate_backup_manifest_covers_derived_tier(backup_manifest, ArchiveTier.INDEX, connection=validate_conn) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Validate the live fingerprint after the mutation lock is held.
Both flows validate index.db before the lock that protects the mutation. A concurrent writer can commit after validation. The subsequent delete or update can then act on an index tier that the backup no longer covers.
polylogue/maintenance/agent_meta_sidecar_purge_apply.py#L201-L201: acquire the write lock used forArchiveStore.delete_sessions, then re-runvalidate_backup_manifest_covers_derived_tierbefore deletion.polylogue/storage/attachment_reacquisition.py#L557-L563: runBEGIN IMMEDIATEbefore the second validation, and retain rollback handling if validation fails.
Add a concurrency regression that changes index.db between precheck and lock acquisition. The apply operation must refuse the stale backup.
📍 Affects 2 files
polylogue/maintenance/agent_meta_sidecar_purge_apply.py#L201-L201(this comment)polylogue/storage/attachment_reacquisition.py#L557-L563
🤖 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 `@polylogue/maintenance/agent_meta_sidecar_purge_apply.py` at line 201, Both
mutation flows revalidate the backup manifest too early; add a post-lock
fingerprint validation. In
polylogue/maintenance/agent_meta_sidecar_purge_apply.py:201, acquire the write
lock used by ArchiveStore.delete_sessions, then rerun
validate_backup_manifest_covers_derived_tier before deletion. In
polylogue/storage/attachment_reacquisition.py:557-563, execute BEGIN IMMEDIATE
before the second validation and preserve rollback handling when validation
fails. Add a concurrency regression that mutates index.db between precheck and
lock acquisition and verifies the apply operation rejects the stale backup.
…ed live Live --apply attempt (branch fix/storage/derived-tier-backup-manifest- attestation, PR #3596) confirms the 5kmn7 attestation fix works: the backup manifest + live index.db fingerprint validation both passed cleanly. Execution then failed safely (no mutation) at ArchiveStore write-mode construction, refused by polylogue-9qnzy's exact schema- currency gap (source.db v15 vs code's declared v20). Wires ioz7 as blocked-by 9qnzy to reflect this confirmed dependency. Co-Authored-By: Claude <noreply@anthropic.com>
CodeRabbit finding on #3596: agent_meta_sidecar_purge_apply.py validated the backup manifest exactly once, before opening the write-mode ArchiveStore -- unlike attachment_reacquisition.py's existing precheck + authoritative-revalidation pattern. A concurrent write to index.db between that single check and the eventual delete_sessions() call would go undetected. ArchiveStore.__init__ acquires the exclusive ActiveWriterLease synchronously for read_only=False, so revalidating right after construction (and before delete_sessions) closes the exact window: nothing else can write once the lease is held. Adds that authoritative revalidation, matching attachment_reacquisition.py's own pattern and comment. Added test_apply_refuses_when_backup_goes_stale_between_precheck_and_write_lease: a fake validator that raises on its second call proves the revalidation is actually wired in and enforced (not merely present as dead code) -- the apply aborts before any mutation, verified via unchanged session ids and zero receipt rows. Updated the existing purges-only-the-phantoms test to expect two validation calls instead of one. attachment_reacquisition.py's own second-validation call already existed before this PR (that's the pattern this change replicates) -- CodeRabbit's finer point about reordering BEGIN IMMEDIATE ahead of that existing validation there is a separate, pre-existing design choice unrelated to the attestation bug this PR fixes; left out of scope. Verification: devtools test on both actuators' test files -- 15 passed. mypy --strict -- clean.
|
Fixed in 0af2eff: added the authoritative revalidation right after ArchiveStore acquires its exclusive writer lease (ArchiveStore.init acquires it synchronously for read_only=False), before delete_sessions runs -- matching attachment_reacquisition.py's existing precheck+revalidation pattern (which is where I copied it from). Added a concurrency regression test (fake validator raises on its second call) proving the revalidation actually aborts the apply before any mutation. Left out of scope: your point about reordering BEGIN IMMEDIATE ahead of the validation in attachment_reacquisition.py itself. That's a pre-existing design choice in code this PR doesn't otherwise touch, not something the attestation bug this PR fixes introduced. |
1 participant
Summary
agent_meta_sidecar_purge_apply.py(polylogue-ioz7) andattachment_reacquisition.pyboth require a verified backup manifest coveringindex.dbbefore mutating it, but the validation call unconditionally requires a cryptographic HMAC attestation for that tier -- an attestation the backup system never produces for derived tiers. No backup manifest, however fresh or complete, could ever satisfy it, so--applywas permanently broken for both actuators.Problem
Discovered running the operator-authorized polylogue-ioz7 live purge against a same-day, all-5-tiers-present,
verdict=successbackup manifest.polylogue/maintenance/agent_meta_sidecar_purge_apply.py:201callsvalidate_migration_backup_manifest(backup_manifest, ArchiveTier.INDEX, connection=validate_conn), which unconditionally requires exactly onetier="index"HMAC attestation entry in the receipt'sattestationslist. Butpolylogue/daemon/backup.py's_write_successful_verification_receipthardcodesauthority_pathsto only ever include{"source", "user"}tiers -- by design, matching the durable-tier architecture where only source.db/user.db are durable and cryptographically attestable; index.db/embeddings.db are derived/rebuildable and were never wired for one.attachment_reacquisition.pyhad the identical bug at two call sites.Solution
Split
migration_runner.py's validation logic into a shared internal helper plus two entry points:validate_migration_backup_manifestkeeps its existing behavior (attestation required) for durable-tier migrations (migrate_archive_tier, unchanged).validate_backup_manifest_covers_derived_tierskips the attestation check for non-durable tiers while keeping every other guarantee: manifest/receipt shape, tier inclusion, receipt/manifest byte match, and a byte-exact live fingerprint (sha256/size/user_version) recomputed from the current on-disk file.Both actuators now call the new function for their
index.dbcheck. Durable-tier callers (raw_authority.py, raw_append_chain_backfill_apply.py, raw_membership_writeback_apply.py, raw_live_source_reconciliation_apply.py) are untouched -- they correctly passArchiveTier.SOURCE.Anti-vacuity
Every existing test for both actuators monkeypatched the validator entirely, so none of them ever exercised a manifest actually produced by
polylogue ops backup --profile full_evidence --verify-- exactly the gap that hid this bug. Addedtest_apply_accepts_a_real_backup_manifest_from_ops_backup, which builds the fixture archive, runs the realbackup_archive()entry point against it, and letsapply_agent_meta_sidecar_purgevalidate that real manifest with no monkeypatching.Verification
devtools test tests/unit/maintenance/test_agent_meta_sidecar_purge_apply.py tests/unit/storage/test_attachment_reacquisition.py-- 14 passed.devtools teston the untouched durable-tier siblings (raw_authority, raw_append_chain_backfill_apply, raw_membership_writeback_apply, raw_live_source_reconciliation_apply, durable_migrations) -- 55 passed, confirming the durable-tier path is unaffected.mypy --stricton all 5 touched files -- clean.devtools verify --quick(pre-push gate) -- clean.Ref polylogue-5kmn7, polylogue-ioz7
Summary by CodeRabbit
Bug Fixes
Tests