feat(storage): dedupe unindexed byte-identical raw quarantine groups - #3697
Merged
Conversation
…up receipts Problem: polylogue-zm4w8 measured 1,777 raw_sessions rows (22.2 GB) among the codex-session quarantine backlog that are pure redundant duplicates -- same source_path AND same blob_hash as another row, with every group member still quarantined (no indexed twin anywhere). raw-byte-duplicate-supersession-apply cannot see this population: it only matches a quarantined raw against an already-INDEXED twin. Solution: additive migration 025 adds raw_quarantine_group_dedup_receipts, mirroring raw_byte_duplicate_supersession_receipts' (migration 023) shape -- revision_authority stays a closed 3-value vocabulary (asserted/byte_proven/ quarantined, no 'superseded' member), so a marked duplicate is promoted to 'byte_proven' with the real evidence (which representative raw/session it matches) recorded in this dedicated receipt table instead. Bumps SOURCE_SCHEMA_VERSION 24 -> 25 and mirrors the DDL into source.py per the durable-tier additive-migration regime.
Problem: polylogue-zm4w8 -- raw-byte-duplicate-supersession's classifier only matches a quarantined raw against an already-indexed twin, so it returns zero candidates for groups where EVERY member is still quarantined (repeated re-acquisitions of the same source file, never materialized). Solution: plan_raw_quarantine_group_dedup groups quarantined raw_sessions rows by (source_path, blob_hash), flags groups with >1 member, and excludes any group whose blob_hash already has an indexed twin or a non-quarantined member anywhere (raw-byte-duplicate-supersession's own territory). The lowest raw_id in a group is the deterministic representative an actuator would materialize; the rest are duplicates. Strictly read-only. Verification: devtools test tests/unit/storage/test_raw_quarantine_group_dedup.py -> 4 passed.
Problem: polylogue-zm4w8's fully-quarantined byte-identical groups (see prior commit's classifier) are real, legitimate content -- repeated acquisitions of the same file -- that were simply never chosen as a representative and materialized. Solution: apply_raw_quarantine_group_dedup mirrors the dry-run-default / --apply-requires-verified-backup-manifest / immutable-receipt pattern every sibling actuator in this family uses, but is genuinely two-phase (unlike the sibling actuators, which are single locked source.db transactions): phase one materializes one representative raw per group through the real production ingest pipeline (ParsingService.parse_from_raw -> write_parsed_session_to_archive -> refresh_session_insights_bulk), re- verified by reading index.db for the resulting sessions row rather than trusted from the ingest call's return value; phase two, only once materialization has genuinely landed, marks the rest of each group 'byte_proven' with a receipt inside a single locked source.db transaction. If materialization produces no indexed session for a group's representative (parse error, refused write, non-session content), that whole group is left untouched rather than guessed at. Wired into devtools workspace raw-quarantine-group-dedup-apply via command_catalog.py; --help verified registered. Verification: devtools test tests/unit/maintenance/test_raw_quarantine_group_dedup_apply.py -> 5 passed (including a full materialize-through-the-real-pipeline happy path). devtools workspace raw-quarantine-group-dedup-apply --help confirms registration.
…n check Problem: polylogue-zm4w8's fully-quarantined byte-identical duplicate groups had no standing regression guard -- the one-shot actuator (prior commit) resolves the current backlog, but nothing catches the pattern recurring. Solution: registers raw-quarantine-group-dedup in ARCHIVE_VERIFICATION_CHECKS (the t0m73 registry pattern), reusing plan_raw_quarantine_group_dedup directly so the check and the actuator can never drift against each other. ERROR when any qualifying group exists; evidence includes scanned/group/ duplicate counts, reclaimable bytes, and a sample of offending groups. Verification: devtools test tests/unit/maintenance/test_archive_verification.py -> 73 passed (3 new: red-twin ERROR case, an already-resolved-elsewhere non-trip case, and the coherent-archive OK case). Read-only dry-run against the live archive (POLYLOGUE_ARCHIVE_ROOT=/realm/db/polylogue) confirms the check fires ERROR with group_count=1822, duplicate_count=1837 (8.22 GiB) as of 2026-08-03 -- close to but not identical to the bead's original 2026-08-03 measurement (1,777 rows / 22.19 GB), consistent with concurrent archive activity since that measurement. No mutation was performed against the live archive.
|
Warning Review limit reached
Next review available in: 36 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (6)
📝 WalkthroughWalkthroughAdds planning and verification for fully quarantined byte-identical raw-session groups. Adds guarded dry-run/apply deduplication, representative ingestion, duplicate marking, immutable receipts, schema migration, tests, and workspace CLI integration. ChangesRaw quarantine group deduplication
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant CLI
participant Deduplication
participant IngestPipeline
participant SourceDB
Operator->>CLI: Run deduplication command
CLI->>Deduplication: Plan or apply groups
Deduplication->>IngestPipeline: Materialize representative
IngestPipeline-->>Deduplication: Create indexed session
Deduplication->>SourceDB: Mark duplicates and write receipts
SourceDB-->>Deduplication: Return integrity result
Deduplication-->>CLI: Return operation report
CLI-->>Operator: Print JSON or text output
Possibly related PRs
Suggested labels: 🚥 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: 7
🤖 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/raw_quarantine_group_dedup_apply.py`:
- Around line 212-239: Ensure phase-one failures do not prevent already
accumulated promotions from reaching phase two: update the group-processing flow
around parse_from_raw, refresh_session_insights_bulk, and the promotions list to
catch failures per group and continue, or execute the corresponding phase-two
promotion immediately after each verified representative. Preserve successful
groups and ensure their duplicates are marked with receipts before the backend
is closed.
- Around line 216-226: Update the representative session lookup in the group
materialization flow around backend.connection() to order results
deterministically and handle every session produced for
group.representative_raw_id. Do not rely on fetchone() selecting an arbitrary
row; collect the ordered session IDs and ensure the receipt records all
materialized sessions and refresh_session_insights_bulk processes each one,
while preserving the existing behavior when no session exists.
- Around line 99-105: Update _checkpoint_live_tier to inspect the first value of
the row returned by PRAGMA wal_checkpoint(TRUNCATE). Treat a busy value of 1 as
a failed checkpoint and raise RawQuarantineGroupDedupApplyError, while
preserving the existing handling for SQLite errors and missing rows.
In `@polylogue/storage/raw_quarantine_group_dedup.py`:
- Around line 193-194: Update the group classification logic in
raw_quarantine_group_dedup so it checks the limit before appending a group,
preventing any group from entering plan.groups when limit=0. Continue processing
classification without adding groups after the cap is reached, preserve
already_resolved_group_count totals, and add a regression test covering limit=0.
In `@polylogue/storage/sqlite/archive_tiers/source.py`:
- Around line 268-270: Update the schema-version marker in the comment above the
immutable receipt table from v23 to v25, matching SOURCE_SCHEMA_VERSION and
migration 025; leave the remainder of the comment unchanged.
In
`@polylogue/storage/sqlite/migrations/source/025_raw_quarantine_group_dedup_receipts.sql`:
- Around line 1-8: The migration’s opening comment contains counts that conflict
with the reported live dry-run figures. Update the documented row, group, and
size measurements to match the live dry run, or explicitly label the existing
and reported figures by their distinct measurement context.
In `@tests/unit/maintenance/test_raw_quarantine_group_dedup_apply.py`:
- Around line 234-260: Add tests in the existing raw quarantine dedup test
module for both safety branches: cover a group whose representative produces no
sessions row and assert the group remains untouched, with no duplicate marking
or receipt; also force phase-two receipt insertion or PRAGMA quick_check
failure, assert the error propagates, and verify revision_authority values and
receipts remain unchanged. Reuse the existing setup and helpers around
test_apply_with_no_qualifying_groups_is_a_clean_no_op and
apply_raw_quarantine_group_dedup.
🪄 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: c99c0152-8ba3-4660-bd1f-ca46d49918b1
📒 Files selected for processing (11)
devtools/command_catalog.pydevtools/raw_quarantine_group_dedup_apply.pydocs/devtools.mdpolylogue/maintenance/archive_verification.pypolylogue/maintenance/raw_quarantine_group_dedup_apply.pypolylogue/storage/raw_quarantine_group_dedup.pypolylogue/storage/sqlite/archive_tiers/source.pypolylogue/storage/sqlite/migrations/source/025_raw_quarantine_group_dedup_receipts.sqltests/unit/maintenance/test_archive_verification.pytests/unit/maintenance/test_raw_quarantine_group_dedup_apply.pytests/unit/storage/test_raw_quarantine_group_dedup.py
Comment on lines
+268
to
+270
| -- v23 (polylogue-zm4w8): one immutable receipt per raw_sessions row marked a | ||
| -- proven duplicate within a fully-quarantined (source_path, blob_hash) group | ||
| -- -- a group where EVERY member starts out quarantined (unlike v22's |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the schema-version marker in the comment.
The comment labels this table v23. This change increments SOURCE_SCHEMA_VERSION to 25 and ships migration 025. The marker misleads a reader who traces a table back to the migration that introduced it.
📝 Proposed fix
--- v23 (polylogue-zm4w8): one immutable receipt per raw_sessions row marked a
+-- v25 (polylogue-zm4w8): one immutable receipt per raw_sessions row marked a
-- proven duplicate within a fully-quarantined (source_path, blob_hash) group📝 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.
Suggested change
| -- v23 (polylogue-zm4w8): one immutable receipt per raw_sessions row marked a | |
| -- proven duplicate within a fully-quarantined (source_path, blob_hash) group | |
| -- -- a group where EVERY member starts out quarantined (unlike v22's | |
| -- v25 (polylogue-zm4w8): one immutable receipt per raw_sessions row marked a | |
| -- proven duplicate within a fully-quarantined (source_path, blob_hash) group | |
| -- -- a group where EVERY member starts out quarantined (unlike v22's |
🤖 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/storage/sqlite/archive_tiers/source.py` around lines 268 - 270,
Update the schema-version marker in the comment above the immutable receipt
table from v23 to v25, matching SOURCE_SCHEMA_VERSION and migration 025; leave
the remainder of the comment unchanged.
Problem: CodeRabbit review on PR #3697 -- plan_raw_quarantine_group_dedup checked the limit cap AFTER appending a group, not before, so limit=0 silently produced one group instead of zero. The apply path iterates plan.groups directly, so a limit=0 dry-run/apply call would still classify, and a limit=0 --apply would still promote and mark, exactly one duplicate group despite the caller asking for none. Solution: check the cap before appending, and continue (not break) so already_resolved_group_count still reflects every remaining already-resolved key, not just the ones seen before the cap was reached. Verification: devtools test tests/unit/storage/test_raw_quarantine_group_dedup.py -> 5 passed (new: test_limit_zero_returns_no_groups).
…review findings Problem: CodeRabbit review on PR #3697 found real correctness gaps in raw_quarantine_group_dedup_apply.py: 1. WAL checkpoint busy flag ignored: PRAGMA wal_checkpoint(TRUNCATE) always returns (busy, log, checkpointed); only a missing row was rejected, so a busy=1 (blocked, WAL not truncated) checkpoint passed silently and a subsequent backup-manifest fingerprint check could attest against a tier with uncheckpointed frames. 2. A phase-one failure left already-materialized groups permanently unreachable: the old two-phase-for-the-whole-plan shape (materialize every representative, then mark every group's duplicates in one batched transaction) meant an exception partway through phase one aborted before phase two ever ran for ANY group -- including groups whose representative had already landed. On the next run those groups' blob_hash already has an indexed twin, so the classifier's own hash_already_resolved exclusion makes them permanently invisible to this actuator. 3. The representative-session lookup had no ORDER BY and used fetchone(), taking an arbitrary row when a raw materializes more than one sessions row (a multi-session capture file) -- the receipt would name an arbitrary session, the rest would never be recorded, and insights would refresh for only one. Solution: - _checkpoint_live_tier now rejects busy=1, not just a missing row. - Materialize-then-mark now runs PER GROUP, not batched across the whole plan (_materialize_group_representative + _mark_group_duplicates): each group's duplicates are marked in their own locked transaction immediately after that group's representative is verified materialized, so an earlier group's success is durably committed before a later group is even attempted. A per-group materialization exception is caught and logged, skipping only that group -- it no longer aborts the run. - The representative lookup now orders by session_id and requires EXACTLY ONE materialized session; zero OR more than one both leave the whole group untouched (documented invariant: this actuator's premise is one raw, one representative session, and a multi-session raw doesn't fit the receipt schema's singular representative_session_id column without inventing ambiguous semantics). Verification: devtools test tests/unit/maintenance/test_raw_quarantine_group_dedup_apply.py tests/unit/maintenance/test_archive_verification.py -> 79 passed (5 new: WAL-busy refusal, zero-session group left untouched, multi-session group left untouched -- empirically verified against the real ingest pipeline that a grouped Claude Code JSONL raw genuinely materializes 2 sessions rows before writing this test, phase-two rollback on a receipt CHECK-constraint violation leaves prior state unchanged). devtools verify --quick -> exit 0.
… counts Problem: CodeRabbit review on PR #3697 (minor findings) -- source.py's inline comment above raw_quarantine_group_dedup_receipts still said "v23" after this PR bumped SOURCE_SCHEMA_VERSION to 25 and shipped migration 025, misleading a reader tracing the table back to its introducing migration. Separately, migration 025's opening comment cited the bead's original filing-time measurement (1,777 rows / 22.2 GiB) without noting the PR's own later, larger live dry-run figure (1,822 groups / 1,837 rows / 8.22 GiB), leaving two different numbers in the permanent record with no indication of which was which. Solution: correct the version marker to v25, and reconcile the migration comment to name both measurements explicitly by what they are (bead-filing- time vs. this PR's later same-day live re-measurement) rather than picking one silently. Verification: devtools verify --quick -> exit 0 (render-all-check, mypy, lint all pass with the corrected comments).
Owner
Author
|
Addressed all CodeRabbit findings (5 the coordinator listed, plus 2 additional Major findings surfaced by re-fetching the full review thread -- fixing those too since they were real correctness gaps):
Also fixed two additional Major findings from the same review thread the initial summary didn't enumerate:
Commits: 1f44160 (limit=0), 747cb0a (WAL busy-flag + per-group materialize/mark + multi-session invariant), d04fafd (comment fixes). Verification: |
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
Adds a permanent archive-verification check plus a one-shot devtools actuator for the raw-authority gap measured in polylogue-zm4w8: quarantined
raw_sessionsrows that are pure byte-identical duplicates of another still-quarantined row sharing the samesource_path, with no indexed twin anywhere -- a populationraw-byte-duplicate-supersession-applycannot see because its own classifier requires an already-indexed twin.Problem
polylogue-zm4w8 measured (2026-08-03, read-only) 1,777 quarantined
codex-sessionraw_sessions rows (22.19 GB) that are redundant re-acquisitions of files already captured by another still-quarantined row -- e.g. one rollout file with nine separate byte-identicalraw_idrows, allrevision_kind='unknown',revision_authority='quarantined'. Confirmed live: zero of these duplicateblob_hashvalues have any non-quarantined twin anywhere inraw_sessions, so the existing actuator (whose classifier's universe is exactly "quarantined rows with an indexed twin") returns zero candidates for this class by construction. This is polylogue-lkrc's simplified scope in its first concrete execution: reclassify the quarantine backlog, verified by the test suite, no manual sorting.Solution
polylogue/storage/raw_quarantine_group_dedup.py: read-only classifier. Groups quarantinedraw_sessionsrows by(source_path, blob_hash), flags groups with >1 member, and excludes any group whoseblob_hashalready has an indexed twin or a non-quarantined member anywhere (that'sraw-byte-duplicate-supersession's territory, not this gap's). The lowestraw_idin a group is the deterministic representative.polylogue/maintenance/raw_quarantine_group_dedup_apply.py+devtools/raw_quarantine_group_dedup_apply.py(wired asdevtools workspace raw-quarantine-group-dedup-apply,--helpverified registered): the actuator. Dry-run by default;--applyrequires--backup-manifest. Unlike every sibling actuator in this family, this one is genuinely two-phase because it must actually materialize content, not just flip a column: phase one promotes exactly one representative raw per group through the real production ingest pipeline (ParsingService.parse_from_raw->write_parsed_session_to_archive->refresh_session_insights_bulk), re-verified by readingindex.dbfor the resultingsessionsrow rather than trusted from the ingest call's return value; phase two, only once materialization has genuinely landed, marks the rest of the grouprevision_authority='byte_proven'inside a single lockedsource.dbtransaction with an immutable receipt (raw_quarantine_group_dedup_receipts, migration 025) pointing at the representative'sraw_id/session_id. If materialization produces no indexed session (parse error, refused write, non-session content), that whole group is left untouched rather than guessed at. Never deletes blobs or runs GC/VACUUM.polylogue/maintenance/archive_verification.py: registersraw-quarantine-group-dedupinARCHIVE_VERIFICATION_CHECKS(the t0m73 registry pattern), reusing the same classifier module directly so the check and the actuator can never drift apart. This is the permanent, ongoing integrity check the bead calls for -- ERROR when any qualifying group exists, part of the ordinaryArchiveVerificationReport.Non-obvious decision: the bead's own design text says "mark the rest
revision_kind='duplicate'/revision_authority='superseded'" -- neither value exists in the schema's closed CHECK vocabularies (revision_kindisfull/append/unknown;revision_authorityisasserted/byte_proven/quarantined), and widening either requires a fullraw_sessionstable rebuild (migration 021's precedent). Instead this reusesrevision_authority='byte_proven'-- the exact mechanismraw-byte-duplicate-supersessionalready established for "quarantined, now proven byte-identical to something real" -- with the actual evidence (which representative raw/session) recorded in a dedicated receipt table, exactly mirroring that actuator's own precedent for the same design tension.Root cause (bead item 3)
Investigated read-only against the live archive (
/realm/db/polylogue, never mutated). Every fully-quarantined byte-identical duplicate group's latest member was acquired on 2026-07-19; zero such groups have any member acquired after that date, even though ordinary ingestion has continued and produced other kinds of raw rows as recently as 2026-07-31 (4 days before this session). This is strong evidence the bug is historical, not live-recurring:deterministic_raw_session_id(storage/sqlite/archive_tiers/source_write.py, present since the #1787 split-file rearchitecture) derivesraw_idfrom(origin, source_path, source_index, blob_hash), so a byte-identical re-acquisition at a stablesource_indexshould already collapse to the sameraw_idrather than mint a new one -- consistent with new duplicate groups no longer appearing. I could not find the exact commit that closed this gap, but the sharp cutoff at 2026-07-19 with continued ingestion afterward is direct evidence against it still recurring for freshly-acquired codex sessions. If the coordinator wants a more precise root-cause commit identification, that's a narrow follow-up, not a blocker for this bead's three-step scope.Verification
devtools test tests/unit/storage/test_raw_quarantine_group_dedup.py tests/unit/maintenance/test_raw_quarantine_group_dedup_apply.py tests/unit/maintenance/test_archive_verification.py-> 79 passed (includes a full happy-path test that materializes a representative raw through the real production ingest pipeline).devtools test tests/unit/storage/test_durable_migrations.py-> 43 passed (schema-version-agnostic assertions absorbed the migration 025 bump cleanly).python3 -m mypy --stricton every touched module (classifier, actuator, devtools CLI, archive_verification.py, source.py, command_catalog.py, and all three test files) -> clean.devtools verify --quick-> exit 0 (format + lint + mypy + render-all-check + layering + closure-matrix + schema/lab policy checks), also ran automatically by the pre-push hook.devtools workspace raw-quarantine-group-dedup-apply --help-> confirms CLI registration.POLYLOGUE_ARCHIVE_ROOT=/realm/db/polylogue devtools workspace raw-quarantine-group-dedup-apply --json, no mutation):group_count=1822,marked_duplicate_count=1837rows (8.22 GiB reclaimable) -- close to but not identical to the bead's original 2026-08-03 measurement, consistent with concurrent archive activity since then.errorwith matching evidence, proving the finding before any fix, per the bead's own step 3.--applywas intentionally never run against the live archive -- left for the coordinator to review the dry-run report and execute with a verified backup manifest, per explicit dispatch instruction.Ref polylogue-zm4w8
Summary by CodeRabbit