fix(storage): stop async raw writer clobbering revision evidence#3099
Conversation
Problem: SQLiteRawMixin.save_raw_session (async_sqlite_raw.py) hand-rolled its own 18-column INSERT OR REPLACE into raw_sessions, while the durable-tier writer (queries/raw_writes.py, used by the production acquire path via RepositoryRawMixin) uses a 28-column INSERT OR IGNORE that also carries revision-authority evidence (logical_source_key, revision_kind, source_revision, predecessor_*, baseline_raw_id, append_*, acquisition_generation, revision_authority). A re-save of an existing raw_id through the async path silently reset every one of those columns to defaults, destroying durable source-tier revision lineage. The mixin also inserted into blob_refs directly, duplicating the separate canonical write_source_raw_session_blob_ref path and bypassing that lifecycle. Solution: delete the hand-rolled SQL and delegate save_raw_session to the single canonical writer (queries/raw_writes.py), matching what RepositoryRawMixin already does. No production caller of the async mixin's method was found (the production acquire path already went through the repository), so this closes the latent clobber without a behavior change for any real caller. Verification: - devtools test tests/unit/storage/test_raw.py tests/unit/storage/test_raw_authority_ledger.py tests/unit/storage/test_duplicate_raw_identity_repair.py -> 60 passed - Anti-vacuity: reverting the source fix while keeping the new tests fails exactly test_async_backend_has_no_second_raw_sessions_insert and test_resave_never_alters_revision_evidence (confirmed via git stash). - devtools test on all other backend.save_raw_session call sites (parsing service, quarantine fixtures, parse tracking, search misc, storage twins, daemon ingest idempotency, ingest pipeline correctness) -> all pass except 6 pre-existing test_parsing_service.py failures unrelated to this change (Mock missing drive_config attribute, reproduces identically on pre-fix code). - devtools verify --quick -> exit 0 (format, lint, mypy --strict, render-all, layering, closure-matrix, schema roundtrip, manifests, ci-workflows, doc-commands, docs-coverage, test-infra-currency, clock-hygiene, timeout-overrides, degrade-loudly). Ref polylogue-vwia 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. |
|
Warning Review limit reached
Next review available in: 27 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 (2)
✨ 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 |
…rtion preservation reads (#3101) ## Summary Closes a latent residual in the assertion write-lock TOCTOU fix (#3051): a caller-owned deferred transaction was never upgraded to hold the SQLite write lock before the terminal-status preservation read. ## Problem `#3051` fixed `upsert_assertion`'s TOCTOU race for the common case by wrapping the preserve/read/write decision in `BEGIN IMMEDIATE` when the connection is idle (`_immediate_user_write_transaction`). But its fallback for an already-open transaction — `if conn.in_transaction: yield; return` — never upgrades that transaction to hold the write lock. A caller-owned deferred (plain `BEGIN`) transaction has not yet reserved the SQLite writer slot, so the terminal-status preservation read taken inside it can still race a concurrent operator judgment landing between that read and this connection's eventual write — reintroducing the exact silent-revert bug `polylogue-41ow` tracks, for any caller that reuses a connection across a transaction it opened itself rather than a fresh connection per call. I audited every production caller of `upsert_assertion`/`judge_assertion_candidate` (`annotations/write.py`, `security/lifecycle.py`, `security/secret_scan.py`, `scenarios/corpus.py`, `storage/repair.py`, `storage/raw_reconciler.py`, and `user_write.py`'s own batch helpers): none currently reach these functions with a caller-owned deferred transaction open, so this exact gap is not yet exploitable in production — but it is latent, one refactor away, the same character as the `vwia` writer-twin bug fixed earlier in this hardening sweep (#3099). ## Solution Closed it defensively. When the connection already has an open transaction, open a `SAVEPOINT` and force a zero-row `UPDATE assertions SET updated_at_ms = updated_at_ms WHERE 0` before yielding — this forces SQLite to acquire the `RESERVED` lock immediately, before any preserve/read decision runs, without committing or rolling back state the caller owns. If the caller's transaction already holds the write lock (the common in-repo case: a batch of `upsert_mark`/`upsert_blackboard_note`/`upsert_saved_view`/`upsert_assertion` calls sharing one `BEGIN IMMEDIATE` opened by the first call in the batch), the forced write is a no-op. Also added the canonical `busy_timeout` PRAGMA to the fresh-transaction branch. Deliberately did **not** port the auto-commit-on-fresh-transaction behavior from an available external delivery patch (`feature/assertions/judgment-transaction`): tracing `scenarios/corpus.py`'s intentional multi-call batch (five upserts sharing one transaction, one final caller commit) confirmed that change would silently split one atomic batch into five separate commits. Also did not port that delivery's non-overlapping additive scope (evidence previews, queue health, capture idempotency, canary script) — that's feature delivery, tracked separately as `polylogue-2o3d`. ## Verification - New regression: `test_cross_connection_replay_inside_caller_owned_deferred_transaction_cannot_resurrect_operator_accept` (`tests/unit/storage/test_archive_tiers_assertions.py`) — two real SQLite connections; the detector opens a plain `BEGIN` (not `IMMEDIATE`) before calling `upsert_assertion`, then a concurrent operator judgment attempts to land between the detector's SELECT and its write. - Anti-vacuity: reverted the source fix via `git stash`, keeping the new test — it fails (operator resurrects the candidate within 0.15s); passes after restoring the fix. - `devtools test tests/unit/storage/test_archive_tiers_assertions.py tests/unit/storage/test_archive_tiers_assertion_write_through.py tests/unit/storage/test_comparative_judgment_assertions.py` → 61 passed - `devtools test` across every other `upsert_assertion`/`judge_assertion_candidate` write-path suite (annotations, security, cli judge/note/candidates, mcp judgment tools, storage user_audit/user_overlay/blackboard/public_claims, scenarios/corpus demo seeding) → 128 passed, no batching regression. - `devtools verify --quick` → exit 0. Ref polylogue-41ow Co-authored-by: Claude <noreply@anthropic.com>
Summary
Deletes the async SQLite backend's hand-rolled
save_raw_sessionINSERT and delegates to the single canonical durable-tier writer, closing a latent revision-evidence clobber.Problem
SQLiteRawMixin.save_raw_session(polylogue/storage/sqlite/async_sqlite_raw.py) hand-rolled its own 18-columnINSERT OR REPLACEintoraw_sessions, while the production writer (polylogue/storage/sqlite/queries/raw_writes.py, reached viaRepositoryRawMixinfrom the real acquire path) uses a 28-columnINSERT OR IGNOREthat also carries revision-authority evidence (logical_source_key,revision_kind,source_revision,predecessor_source_revision,predecessor_raw_id,baseline_raw_id,append_start_offset,append_end_offset,acquisition_generation,revision_authority). A re-save of an existingraw_idthrough the async path would silently reset all of those columns to defaults on the durable source tier, destroying revision lineage. The mixin also wrote directly intoblob_refs, duplicating the separate canonicalwrite_source_raw_session_blob_refpath and its lifecycle.No production caller of the async mixin's method was found — the real acquire path (
AcquisitionService→persist_raw_record→SessionRepository.save_raw_session) already goes throughRepositoryRawMixin, which already delegates to the correct writer. The clobber was latent (reachable only via directbackend.save_raw_sessioncalls, as many tests do), one refactor away from being wired into production.Solution
Deleted the hand-rolled SQL body and made
SQLiteRawMixin.save_raw_sessiondelegate toqueries/raw_writes.save_raw_session, matching whatRepositoryRawMixin.save_raw_sessionalready does — single source of truth for the INSERT, no possibility of column-list divergence. Removed now-unused imports (hashlib,origin_from_provider,_timestamp_ms) and the now-dead_source_db_pathtype stub.Added two regression tests in
tests/unit/storage/test_raw.py:test_writer_insert_covers_every_live_raw_sessions_column: introspects the liveraw_sessionsDDL viaPRAGMA table_infoand asserts the canonical writer's INSERT column list matches exactly — fails if a future migration adds a column the writer doesn't reference, or vice versa.test_async_backend_has_no_second_raw_sessions_insert: fails if a hand-rolledINTO raw_sessionsINSERT reappears in the async mixin.test_resave_never_alters_revision_evidence: saves a record with populatedRawRevisionEnvelope(byte-proven authority), re-saves the sameraw_idwith a bare record, and asserts the original revision evidence survives unchanged.Verification
devtools test tests/unit/storage/test_raw.py tests/unit/storage/test_raw_authority_ledger.py tests/unit/storage/test_duplicate_raw_identity_repair.py→ 60 passedgit stash— exactlytest_async_backend_has_no_second_raw_sessions_insertandtest_resave_never_alters_revision_evidencefailed; the DDL-parity test passed (it targetsraw_writes.py, which was already correct).devtools teston every otherbackend.save_raw_sessioncall site (parsing service, quarantine fixtures, parse tracking, search misc, storage twins, daemon ingest idempotency, ingest pipeline correctness) → all pass except 6 pre-existingtest_parsing_service.pyfailures (Mock has no attribute 'drive_config'), confirmed to reproduce identically on pre-fix code via the same stash technique — unrelated to this change.devtools verify --quick→ exit 0 (format, lint, mypy --strict, render-all, layering, closure-matrix, schema roundtrip, manifests, ci-workflows, doc-commands, docs-coverage, test-infra-currency, clock-hygiene, timeout-overrides, degrade-loudly).Ref polylogue-vwia