fix(storage): write.py identity companions + periodic FTS drift stage#3239
Merged
Sinity merged 3 commits intoJul 21, 2026
Merged
Conversation
…-miwv) Problem polylogue-1xc.12 (PR #3235) added the messages_fts_identity shadow ledger and paired it with every messages_fts bulk-write call site except write.py's non-bulk full-session-replace fast path, which calls delete_session_rows_sql/insert_session_rows_sql directly. Sessions written through the dominant real-world write path (write_parsed_session_to_archive with merge_append=False, the default) were coverage-incomplete: messages_fts_docsize had a row but messages_fts_identity never did, until the next repair pass backfilled it. The 1xc.12 lane STOP-and-reported this because write.py is a restricted hot file. Solution Pair every delete_session_rows_sql/insert_session_rows_sql call in write.py with the matching delete_session_identity_rows_sql/ insert_session_identity_rows_sql call, same chunk params, at all three sites: the scoped fast-path delete+reinsert in _replace_full_session_messages_and_blocks, the bulk-fts-guard delete+reinsert in _suspend_message_fts_for_session, and the trigger-missing purge in _purge_session_message_fts_when_delete_trigger_missing (delete-only; paired even without a matching insert since the blocks it covers are deleted right after, so an unpaired delete would orphan ledger rows). Also paired the sync/async twin purge helpers in storage/session_replacement.py (_purge_message_fts_sync/_async), which have the identical delete-before-block-delete shape but were not covered by the 1xc.12 lane's write.py count. Updated message_identity_mismatch_sql's docstring and the v43 section of docs/internals.md to reflect the closed gap (the "missing entry is not a conflict" design boundary itself is unchanged, kept as defense-in-depth for pre-1xc.12 archive history). Verification - devtools test tests/unit/storage/test_fts_identity_ledger.py -> 18 passed (adds TestWritePathIdentityCompanions: exercises the real write_parsed_session_to_archive entry point through a genuine full-session-replace and a merge-append control, asserting both message_identity_mismatch_sql()==0 and a direct messages_fts_docsize-LEFT-JOIN-messages_fts_identity missing-entry count ==0) - devtools test tests/unit/storage/test_fts_bloat_invariants.py tests/property/test_fts_identity_state_machine.py -> 6 passed - devtools test tests/unit/storage/test_archive_tiers_write.py tests/unit/storage/test_bulk_fts_prefix_reextract.py tests/unit/storage/test_bulk_build_lifecycle.py tests/unit/storage/test_lineage_normalization.py -> 107 passed - mypy --strict polylogue/storage/sqlite/archive_tiers/write.py polylogue/storage/session_replacement.py tests/unit/storage/test_fts_identity_ledger.py -> Success, no issues - Anti-vacuity: manually reverted the two fast-path companion calls in _replace_full_session_messages_and_blocks and re-ran test_full_session_replace_leaves_zero_missing_identity_entries -> FAILED (assert 2 == 0); restored the fix and re-ran -> passed Ref polylogue-miwv Co-Authored-By: Claude <noreply@anthropic.com>
Problem messages_fts_identity carries two independent uniqueness constraints (the rowid INTEGER PRIMARY KEY and the block_id TEXT UNIQUE column), but the messages_fts_ad/au trigger arms and every bulk delete companion in storage/fts/sql.py only ever clean up ledger rows by rowid. Any path that leaves a stale (old_rowid, block_id=X) ledger row behind -- a delete that ran with FTS triggers/companions suppressed and no matching cleanup for that exact rowid -- means a later write that computes block_id X at a different rowid hits `UNIQUE constraint failed: messages_fts_identity.block_id` and aborts the write outright. Reported by another lane as a live pre-existing regression from #3235 (messages_fts_identity's introduction), surfacing under xdist as failures in tests/unit/storage/test_revision_replay.py and tests/unit/sources/test_live_batch_support.py. The exact production write-sequence that produces the orphan was not reproducible: both flagged files pass cleanly and repeatedly (serial, `-n auto` xdist, 3+ runs each) against this branch and a disposable fresh `origin/master` worktree checkout -- the only failure present in either is a pre-existing, unrelated `test_bundle_replay_respects_ unconvertible_single_session_head` file-cleanup assertion, confirmed present on unmodified origin/master too. A synthetic repro built directly against the real trigger DDL (drop the AD trigger, delete a block so its ledger row orphans, restore triggers, insert a new block that computes the same block_id at a different rowid) reliably reproduces `sqlite3.IntegrityError: UNIQUE constraint failed: messages_fts_identity.block_id` on current code, confirming the invariant gap independent of the unreproduced call sequence. Solution Every place messages_fts_identity is written now uses INSERT OR REPLACE (the messages_fts_ai/au trigger bodies, insert_all_message_ identity_rows_sql, insert_session_identity_rows_sql) instead of a bare INSERT, so a pre-existing row blocking on EITHER constraint (rowid or block_id) is evicted before the incoming row lands -- the ledger self-heals instead of aborting the write. repair_message_identity_ rows_range_sql keeps its original ON CONFLICT(rowid) DO UPDATE (with the existing no-op-avoiding WHERE, preserving the batched-repair commit-skip optimization) and gains a second ON CONFLICT(block_id) DO UPDATE clause -- SQLite's multi-target UPSERT (3.35+; this project already requires 3.43+ for FTS5 contentless_delete) -- to move a block_id's identity onto the correct rowid instead of raising. Verification - Synthetic repro script against the real trigger DDL: IntegrityError before the fix, clean self-heal after (final ledger row correctly at the new rowid, stale rowid entry evicted). - devtools test tests/unit/storage/test_fts_identity_ledger.py -> 19 passed (new TestIdentityLedgerBlockIdCollisionRepro test constructs a genuine orphaned ledger row via the real messages_fts_ad trigger drop + block delete + trigger restore, then inserts a colliding block_id at a new rowid through the real messages_fts_ai trigger) - Anti-vacuity: manually reverted the messages_fts_ai trigger's INSERT OR REPLACE back to a bare INSERT and reran the new test -> FAILED (sqlite3.IntegrityError); restored the fix and reran -> passed - devtools test tests/unit/storage/test_revision_replay.py tests/unit/sources/test_live_batch_support.py tests/unit/storage/test_fts_bloat_invariants.py tests/property/test_fts_identity_state_machine.py tests/unit/storage/test_archive_tiers_write.py tests/unit/storage/test_bulk_fts_prefix_reextract.py tests/unit/storage/test_bulk_build_lifecycle.py tests/unit/storage/test_lineage_normalization.py tests/unit/daemon/test_fts_identity_convergence.py -> 205 passed, 2 failed (the pre-existing unrelated bundle-replay-cleanup assertion, confirmed present on unmodified origin/master too) - mypy --strict polylogue/storage/fts/sql.py tests/unit/storage/test_fts_identity_ledger.py -> Success, no issues Ref polylogue-miwv Co-Authored-By: Claude <noreply@anthropic.com>
Problem The messages_fts_identity ledger (polylogue-1xc.12) self-heals inline on every ordinary write path and full reconciliation (fts_invariant_snapshot_sync) already runs during rebuild, batched repair, and daemon startup -- but none of those call sites is a scheduled, quiet-cadence pass whose sole job is recomputing the exact snapshot and re-recording it. That matters because daemon/fts_ startup.py's bounded STALE-write path (_ensure_archive_messages_fts_startup_readiness_sync) calls record_fts_surface_state_sync with only its bounded source_rows/ indexed_rows/missing_rows/excess_rows counts, never identity_mismatch_rows -- the parameter defaults to 0 and the ON CONFLICT upsert overwrites the freshness row unconditionally, so a previously recorded nonzero identity_mismatch_rows silently resets to 0 without ever being recomputed. Solution Added polylogue/daemon/fts_identity_convergence.py: a periodic asyncio loop (periodic_fts_identity_drift_recompute) scheduled directly in daemon/cli.py alongside the other maintenance loops, on a fixed 24-hour quiet cadence matching _periodic_db_optimize's reasoning (full-archive exact reconciliation is the same cost class as PRAGMA optimize's planner-stat refresh, not a hot-path check) -- no new config knob. Gated on the same catch_up_complete event other periodic loops use so it never races initial source catch-up. Deliberately NOT a new DaemonConverger/ConvergenceStage: read docs/retro/2026-05-24-1498-cascade.md first per this bead's instructions -- its explicit verdict on convergence_stages.py is "refactor before adding a fourth stage" (~1,100 lines accreted through the #1498 cascade). Instead follows the periodic_judgment_automation_ sweep (polylogue-6qjc, PR #3229) shape: a standalone module with a sync run-once function plus an async scheduling loop, wired into daemon/cli.py's periodic_loops list. The sync body (run_fts_identity_drift_recompute_once_sync) always calls the real fts_invariant_snapshot_sync + record_fts_invariant_ snapshot_sync + sample_fts_drift_to_ops_sync composition -- the same three-call sequence already used by fts_lifecycle.py's rebuild/repair paths and fts_startup.py's successful-startup path -- making it the recompute authority that corrects any stale identity_mismatch_rows a narrower bounded caller left behind. Verification - devtools test tests/unit/daemon/test_fts_identity_convergence.py -> 4 passed: a corrupted-ledger-row/repair round trip proving the stage recomputes nonzero then zero against the real writer + real reconciliation SQL; a dedicated reproduction of the exact documented hazard (record_fts_surface_state_sync called without identity_mismatch_rows resets the recorded count to 0, then the next recompute pass restores the true nonzero count); a missing-db bounded no-op case; and an ops.db drift-sample anti-vacuity check proving all three composed functions actually ran, not a stub. - mypy --strict polylogue/daemon/fts_identity_convergence.py polylogue/daemon/cli.py tests/unit/daemon/test_fts_identity_convergence.py -> Success, no issues - devtools render topology-projection && devtools render topology-status (new module under polylogue/) -> committed docs/plans/topology-target.yaml + docs/topology-status.md - devtools render all --check -> no "out of sync" surfaces Ref polylogue-miwv Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 36 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 Run ID: 📒 Files selected for processing (10)
✨ Finishing Touches🧪 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 |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Owner
Author
|
Coordinator stand-in review (CodeRabbit rate-limited, 0 inline comments): read the full 1076-line diff.
Merging. |
Sinity
deleted the
feature/storage/fts-identity-write-path-and-periodic-stage
branch
July 21, 2026 13:12
Sinity
added a commit
that referenced
this pull request
Jul 21, 2026
…unmasked) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QUsH3Rhq6oAZpYPWcsZqnZ
Sinity
added a commit
that referenced
this pull request
Jul 21, 2026
) ## Summary Fixes 2 test failures reported on current master (`test_bundle_replay_respects_unconvertible_single_session_head[bundle_texts2-False-False]` and `[bundle_texts3-False-True]` in `tests/unit/sources/test_live_batch_support.py`). Root cause is **not** related to `messages_fts_identity`/polylogue-miwv's earlier work (PR #3239) — diagnosis below. ## Problem The working hypothesis (from another lane) was that PR #3239's `messages_fts_identity` `INSERT OR REPLACE` fix removed an `IntegrityError` abort that had been accidentally masking this test failure. That hypothesis is **disproven**: the exact same failure reproduces identically on commit `b3429fae6` (#3234), the commit immediately *before* `messages_fts_identity` (#3235) was even introduced. Bisecting by direct commit checkout (not `git stash`) traced the real regression to PR #3211 ("tree-byte decoded layer + in-cohort head-retire drift fix", 2026-07-20, ~10.5h before #3235). That PR removed a byte-governance refusal from `ArchiveStore.apply_raw_membership_classification` on the premise that its branch is only reachable after a real membership-governance conversion — but `_apply_membership_sessions` (`sources/live/batch.py`) unconditionally injects the *current* accepted head into the comparison cohort even when it was never converted (exactly PR #2718's original scenario: a byte-governed head compared against membership-discovered content for the first time). With the guard gone, a later-discovered bundle raw whose content happened to strictly extend the byte-governed head's content silently replaced the head via membership governance — confirmed with a standalone diagnostic script: `message_count` moved 2→3, `accepted_raw_id` changed, even though the head still had a live, unresolved `QUARANTINED` append raw blocking ordinary byte-chain conversion. The fail-closed contract was genuinely violated, not merely a reporting-list discrepancy. #3211 shipped this bundled into an unrelated perf PR without running `test_live_batch_support.py` in its own verification. ## Solution Restored a byte-governance refusal in `apply_raw_membership_classification`, narrower than #2718's original blanket `logical_source_key IS NOT NULL` check so #3211's own interrupted-pass-drift resumption keeps working: refuse only when (a) replay is about to *change* which raw is accepted, and (b) a live `raw_sessions` row elsewhere in this logical identity still chains a `predecessor_source_revision` off the existing head's own `source_revision` and isn't already part of the classified cohort — genuine unresolved byte-append evidence, not merely "was this raw ever membership-converted". ## Verification - Bisected via direct commit checkout: reproduced on `b3429fae6` (pre-#3235); traced the introducing diff to `996a3d6d3` (#3211) via `git log -S` on the removed guard string. - `devtools test tests/unit/sources/test_live_batch_support.py -k test_bundle_replay_respects_unconvertible_single_session_head` → 4 passed (all parametrizations, including the `succeeds=True` case) - `devtools test tests/unit/sources/test_live_batch_support.py tests/unit/storage/test_revision_replay.py tests/unit/sources/test_revision_backfill.py` → 136 passed - `devtools test tests/unit/storage/test_raw_retention.py` → 59 passed - `mypy --strict polylogue/storage/sqlite/archive_tiers/archive.py tests/unit/sources/test_live_batch_support.py` → Success, no issues - `devtools verify --quick` → exit 0 (also regenerated `docs/topology-status.md`, already out of sync on unmodified `origin/master` before this branch — unrelated pre-existing drift, picked up incidentally) - Anti-vacuity: reverted just the new guard body to a no-op and reran the 4-case test → 2 of 4 failed with the exact original symptom; restored and reran → 4 passed ## AC matrix | AC | Status | | --- | --- | | Diagnose true mechanism (bisect between the 3 merged changes) | Satisfied — root cause is #3211, not messages_fts_identity/miwv | | Determine whether content actually lands or reporting-only | Satisfied — content genuinely landed before this fix | | Fix production path to preserve explicit refusal | Satisfied | | `head_after == head_before` + `message_count` stays `(2,)` for `succeeds=False` | Satisfied (already asserted by the pre-existing test, now passing) | Ref polylogue-miwv Co-authored-by: Claude <noreply@anthropic.com>
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
Implements the two named residuals from polylogue-1xc.12 (PR #3235), plus fixes a live pre-existing
messages_fts_identityUNIQUE(block_id) regression reported mid-flight by another lane:storage/sqlite/archive_tiers/write.py's non-bulk full-session-replace fast path'sdelete_session_rows_sql/insert_session_rows_sqlcalls with the matchingdelete_session_identity_rows_sql/insert_session_identity_rows_sqlcompanions (3 delete + 2 insert sites, plus the sync/async twin purge helpers instorage/session_replacement.py).polylogue/daemon/fts_identity_convergence.py: a periodic 24h-cadence recompute pass that is the recompute authority foridentity_mismatch_rows, correcting the documenteddaemon/fts_startup.pybounded-STALE-write hazard.messages_fts_identityblock_id UNIQUE-constraint self-heal gap: every identity-ledger write now usesINSERT OR REPLACE/multi-target UPSERT so a stale orphaned ledger row can never abort a later colliding write.Problem
AC2 residual (write.py):
message_identity_mismatch_sql's docstring documented that write.py's fast path did not call the identity companions inline, leaving sessions written through the dominant real-world path (write_parsed_session_to_archive,merge_append=False) coverage-incomplete until the next repair backfilled them.AC4 residual (periodic stage): identity self-heal ran on every rebuild/repair/startup but no scheduled pass recomputed the exact snapshot on a quiet cadence, and
fts_startup.py's bounded STALE-write path could reset a recorded nonzeroidentity_mismatch_rowsto 0 without recomputing it.Live regression (mid-flight, coordinator-assigned): another lane reported 6 pre-existing failures on unmodified master tied to
messages_fts_identity'sblock_id TEXT NOT NULL UNIQUEconstraint, intests/unit/storage/test_revision_replay.pyandtests/unit/sources/test_live_batch_support.py. The exact production write-sequence was not reproducible: both files pass cleanly and repeatedly (serial,-n autoxdist, multiple runs) against this branch and a disposable freshorigin/masterworktree — the only failure present in either is a pre-existing, unrelatedtest_bundle_replay_respects_unconvertible_single_session_headfile-cleanup assertion, confirmed present on unmodifiedorigin/mastertoo. However, a synthetic repro built directly against the real trigger DDL (orphan a ledger row via a trigger-suspended delete, then insert a new block computing the sameblock_idat a different rowid) reliably reproducessqlite3.IntegrityError: UNIQUE constraint failed: messages_fts_identity.block_idon current code, confirming a genuine invariant gap independent of the unreproduced call sequence: the ad/au trigger arms and every bulk delete companion only ever clean up ledger rows byrowid, never byblock_id.Solution
session_replacement.py.fts_identity_convergence.py: standalone module (not a newDaemonConverger/ConvergenceStage—docs/retro/2026-05-24-1498-cascade.md's explicit verdict is "refactor before adding a fourth stage"), following theperiodic_judgment_automation_sweep(PR feat(daemon): add judgment-automation policy sweep actor #3229) shape: a sync run-once function + async scheduling loop wired intodaemon/cli.py'speriodic_loops, fixed 24h cadence (matching_periodic_db_optimize's cost-class reasoning), no new config knob.messages_fts_identitywrites now useINSERT OR REPLACE(trigger bodies,insert_all_message_identity_rows_sql,insert_session_identity_rows_sql) and a secondON CONFLICT(block_id)clause onrepair_message_identity_rows_range_sql(SQLite 3.35+ multi-target UPSERT; project already requires 3.43+ for FTS5contentless_delete), so a pre-existing row blocking on either therowidPK or theblock_idUNIQUE constraint is evicted before the incoming row lands.Verification
devtools test tests/unit/storage/test_fts_identity_ledger.py→ 19 passed (newTestWritePathIdentityCompanions+TestIdentityLedgerBlockIdCollisionReproclasses)devtools test tests/unit/daemon/test_fts_identity_convergence.py→ 4 passeddevtools test tests/unit/storage/test_revision_replay.py tests/unit/sources/test_live_batch_support.py tests/unit/storage/test_fts_bloat_invariants.py tests/property/test_fts_identity_state_machine.py tests/unit/storage/test_archive_tiers_write.py tests/unit/storage/test_bulk_fts_prefix_reextract.py tests/unit/storage/test_bulk_build_lifecycle.py tests/unit/storage/test_lineage_normalization.py tests/unit/daemon/test_fts_identity_convergence.py→ 205 passed, 2 failed (pre-existing unrelated bundle-replay-cleanup assertion, confirmed present on unmodifiedorigin/master)mypy --stricton all touched/new modules → Success, no issuesdevtools render topology-projection && devtools render topology-status(new module) → committeddevtools render all --check→ no "out of sync" surfacesdevtools verify --quick→ exit 0INSERT OR REPLACEfix and reran the corresponding tests → both failed as expected (missing-entry assertion /sqlite3.IntegrityError); restored and reran → both passedAC matrix
Ref polylogue-miwv