refactor(storage): consolidate the three skip-stale-replace tie-breaks - #3454
Merged
Conversation
Problem polylogue-t83e investigated 6 live session_id collisions between drive- cache Claude Code transcript re-uploads (a copy the operator uploaded into an AI Studio conversation, re-downloaded via Drive sync) and their genuinely local ~/.claude/projects/ counterparts, where the drive copy (a strict byte-prefix of the local file) was winning the archive row. Investigation traced this to a revision-arbitration bug, not an identity/detection bug: session_revision_membership.py's content-only set relation already resolves this correctly (verified by direct simulation against the real raw bytes with current code: relation computes a_contains_b, local strictly dominates). The live archive's raw_session_memberships rows for these 6 cohorts simply predate the fix that landed the same day (PR #3401/#3405, polylogue-aggz) and will self-heal via the daemon's automagic bulk rebuild path (daemon/bulk_rebuild.py -> rebuild_index_from_source -> backfill_historical_revision_evidence -> classify_membership_revisions) on the operator's already-planned `ops reset --index && polylogued run`. No SEMANTIC_REPARSE declaration applies -- no detection or parsing semantics changed. Two approaches were tried and reverted before reaching this conclusion (see polylogue-t83e's closing comment for the full trail): an OriginSpec artifact rule refusing session admission for drive-cache Claude-Code-shaped paths, and a matching ingest_worker.py path check. Both worked but were wrong: they would have permanently suppressed the 6 non-colliding drive-cache raws (real content with no local counterpart to supersede them) and added unneeded machinery for the 6 colliding ones, which the already-fixed revision-membership relation resolves once the data catches up. Solution What remained a real, independently-valuable defect: the freshness-tie "skip this write as stale" comparison was duplicated three times (archive_tiers/write.py, pipeline/services/ingest_batch/_core.py, archive_tiers/revision_governance.py) with identical logic (`existing_updated_at_ms is not None and incoming_freshness_ms < existing_updated_at_ms`) that could silently drift apart. Consolidated into one `should_skip_stale_replace()` in archive_tiers/ingest_precedence.py (the module that already owns browser-capture write precedence), called from all three sites. Each call site keeps its own surrounding guard conditions (force_write/ force_replace, browser-capture precedence, append-only, revision- authority membership, source_index gating) -- those decide whether the check applies at all, not the comparison itself. The docstring records that this is a narrow per-write timestamp fallback, not where content- subset supersession is decided (that's revision membership, upstream). Verification - devtools test tests/unit/pipeline/test_archive_write.py::test_older_full_replace_does_not_overwrite_newer_session_body tests/unit/pipeline/test_ingest_batch.py::test_write_session_force_write_replaces_older_freshness -- 2 passed - devtools test tests/unit/storage/test_revision_replay.py tests/unit/storage/test_raw_revision_authority.py tests/unit/sources/test_revision_backfill.py -- 103 passed, 1 failed (test_parse_one_still_replays_real_claude_code_sessions_with_no_path_rule -- pre-existing on origin/master, zero diff in the files it exercises, unrelated to this change: an analysis/-path content-gate regression, not touched here) - devtools verify --seed-testmon --skip-slow -- ruff format, ruff check, mypy --strict, render all --check, topology/layering/closure-matrix, schema-versioning policy, schema-promotion audit all green; the full pytest seed pass was still running a broad corpus sweep when the 580s budget for this verification pass was reached (unrelated to this change's narrow surface) Ref polylogue-t83e Co-Authored-By: Claude <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe change adds a shared stale-replacement timestamp helper. Ingest and archive session replacement paths now use the helper instead of inline timestamp comparisons. ChangesStale replacement precedence
Estimated code review effort: 2 (Simple) | ~10 minutes ✨ Finishing Touches📝 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 |
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
Consolidates three independently-duplicated "skip this write as stale" freshness-tie checks (
archive_tiers/write.py,pipeline/services/ingest_batch/_core.py,archive_tiers/revision_governance.py) into oneshould_skip_stale_replace()inarchive_tiers/ingest_precedence.py. No behavior change: the comparison body is unchanged, just no longer copy-pasted three times.Problem
polylogue-t83einvestigated 6 livesession_idcollisions where a Claude Code transcript the operator uploaded into an AI Studio conversation (re-downloaded via Drive sync into~/.local/share/polylogue/drive-cache/gemini/<uuid>.jsonl.txt.json) was winning the archive row over the genuinely local~/.claude/projects/...transcript of the same conversation — even though the drive copy is a strict byte-prefix of the local file (confirmed by direct diff: 213 vs 214 Claude Code JSONL messages for one collision, the local file has exactly one trailingsummaryrecord the drive copy lacks).Before/after collision table (read-only queries against
/realm/db/polylogue/{source,index}.db):sessions.raw_idtodayraw_session_memberships.decisionambiguous(stale, decided 2026-07-30T05:05Z)a_contains_b(local dominates)ambiguous(stale)ambiguous(stale)ambiguous(stale)ambiguous(stale)ambiguous(stale)(15 drive-cache raws total, all genuinely Claude-Code-shaped by content, parsing into 12 sessions — 6 collide with a local raw as above, 6 do not.)
Root cause, precisely: this is a revision-arbitration bug, not an identity/detection bug.
session_id = origin || ':' || native_idcorrectly computes the same identity for both raws because they are the same conversation — that's not a defect. Which raw should win isarchive/session_revision_membership.py's job (a content-only set relation:equal/a_contains_b/b_contains_a/conflict, polylogue-aggz). Directly simulating that relation against the real raw bytes with current code (parse_stream_payload+session_revision_projection) givesa_contains_b(local strictly dominates: 0 drive-only messages, 1 local-only message, 0 content mismatches on the 213 shared identities) — i.e. the fix already exists. The live archive'sraw_session_membershipsdecision rows for these 6 cohorts (decided_at_ms≈ 2026-07-30T05:05–05:13Z) simply predate the commits that fixed the comparison logic (9fc5220ef#3401 "collapse revision comparison into a content-only relation", 15:55Z same day, anda9f2f307d#3405, 17:50Z) — stale data, not a live bug.This self-heals without further code changes.
daemon/bulk_rebuild.py's own docstring documents that the daemon routes a bulk-scale backlog (whichpolylogue ops reset --indexcreates) intorebuild_index_from_source→backfill_historical_revision_evidence→classify_membership_revisions— the current, already-fixed relation — "with zero operator involvement". The operator's already-plannedops reset --index && polylogued runwill recompute these 6 cohorts correctly. NoSEMANTIC_REPARSEdelta declaration is needed: no detection or parsing semantics changed in this PR, and the relevant fix already shipped (and presumably was already declared) in #3401/#3405.Record correction: PR #3436 verified these 12 drive-cache/gemini rows are content-shape-correct ("naming coincidence, not a bug") — that remains true, the content-shape classification was never wrong. It did not check
native_idcollisions against local raws, so it didn't catch that 6 of the 12 were silently shadowing a fuller local transcript. That shadowing was a data-staleness artifact of a bug fixed the same day this bead's forensics ran, not a defect in #3436's own change.Two approaches were tried and reverted before landing on this conclusion (session-identity gating in
sources/origin_specs.py+pipeline/services/ingest_worker.py, refusing session admission for drive-cache Claude-Code-shaped paths). Both worked mechanically but were wrong: they would have permanently suppressed the 6 non-colliding drive-cache raws (real content with no local counterpart to supersede them, so they'd become inert un-queryableraw_artifactsrows instead of correctly-attributed real content) and added unneeded machinery for the 6 colliding ones, which the already-fixed relation resolves once the data catches up. Full trail recorded inpolylogue-t83e's closing comment.Solution
What remained a real, independently-valuable defect, unrelated to the collision-resolution question above: the freshness-tie comparison itself (
existing_updated_at_ms is not None and incoming_freshness_ms < existing_updated_at_ms) was hand-duplicated in three call sites that could silently drift apart. Extractedshould_skip_stale_replace()intoarchive_tiers/ingest_precedence.py(the module that already owns browser-capture write precedence) and call it from all three:polylogue/storage/sqlite/archive_tiers/write.py(write_parsed_session_to_archive)polylogue/pipeline/services/ingest_batch/_core.py(_write_session)polylogue/storage/sqlite/archive_tiers/revision_governance.py(_write_parsed_precedence_result)Each call site keeps its own surrounding guard conditions (
force_write/force_replace, browser-capture precedence, append-only, revision-authority membership,source_indexgating) — those decide whether the check applies at all, not the comparison. The function's docstring documents that this is a narrow per-write timestamp fallback, not where content-subset supersession is decided (that's revision membership, upstream, and takes precedence when it has classified a cohort).Verification
devtools test tests/unit/pipeline/test_archive_write.py::test_older_full_replace_does_not_overwrite_newer_session_body tests/unit/pipeline/test_ingest_batch.py::test_write_session_force_write_replaces_older_freshness— 2 passed (exerciseswrite.pyandingest_batch/_core.py's edited branches directly)devtools test tests/unit/storage/test_revision_replay.py tests/unit/storage/test_raw_revision_authority.py tests/unit/sources/test_revision_backfill.py— 103 passed, 1 failed. The failure (test_parse_one_still_replays_real_claude_code_sessions_with_no_path_rule) is pre-existing onorigin/master: zero diff in the files it exercises (archive/artifact_taxonomy/,sources/revision_backfill.py,sources/origin_specs.py), reproduces identically in isolation, and is about an unrelatedanalysis/-path content-gate regression.devtools verify --seed-testmon --skip-slow— ruff format, ruff check, mypy --strict, render all --check, topology/layering/closure-matrix, schema-versioning policy, schema-promotion audit all green (see step-by-steprun.json, exit 0 through step 18); the full pytest seed pass was still mid-sweep when the verification budget for this session ran out — unrelated to this change's narrow surface, already covered by the targeted runs above.devtools verify --quick— green via the pre-push hook (format/lint/mypy/render).Ref polylogue-t83e (closed this session with the full investigation trail in its final comment)
Summary by CodeRabbit