Persist ingest invariant verification metadata - #1030
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
General PR Review: Persist ingest invariant verification metadataBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe full PR diff was scanned for security and correctness. This commit hardens the verification marker against prior review findings: MarkIngestInvariantsVerified now refuses unfinished syncs in both the SQLite ended_at-is-not-null predicate plus FailedPrecondition and Pebble GetEndedAt-nil guard paths; a pebbleStore.SyncMeta override flips the dirty bit so a standalone metadata stamp on a reopened c1z survives Close; stale markers are cleared at sync start before any collection write; persistIngestInvariantVerification failure is now log-only, fail-closed to unverified rather than failing a sealed sync; and ToPebble no longer converts an unfinished-but-marked source into a sealed verified destination. Proto source matches the regenerated pb output, and the SQLite read path short-circuits on empty generation so a cleared marker never triggers a malformed-JSON parse. No new issues found. Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
A crash between the invariant pass and the final checkpoint/EndSync left the marker readable on an UNFINISHED sync: the resume machinery pushes InitOp and re-collects over the same sync while the stale marker claims the data verified, and any consumer reading the crash image meanwhile would trust an artifact that never sealed. runIngestionInvariants now only stages the verification on the syncer; persistIngestInvariantVerification writes it after EndSync succeeds. Both built-in writers accept metadata stamps on a sealed sync (SQLite updates by sync_id; Pebble's PutSyncRunRecord is AllowSealed). The inverse crash window — sealed but not yet marked — reads as an unverified legacy artifact: fail-closed. TestCrashBetweenInvariantPassAndSealLeavesNoMarker pins the promise on both engines: crash at haltStageInvariantsComplete leaves the unfinished sync unverified, and the resumed run seals and marks it. Co-authored-by: Cursor <cursoragent@cursor.com>
Four fixes from the second adversarial review round: 1. Marker persistence after EndSync is log-only. EndSync stays the last mutation that can fail the sync; a Mark failure on a sealed, complete artifact must not read as a FAILED sync (IsSyncPreservable callers would discard the artifact and a retry would re-collect from scratch over it). 2. pebbleStore overrides SyncMeta() with a dirty-marking wrapper. Mark/Clear (and MarkSyncSupportsDiff / RecalculateStats) previously wrote to the engine without flipping the store's dirty bit, so a standalone metadata stamp on a reopened c1z was silently dropped at Close. This also made the fold test's inherit-nothing assertions vacuous: markFoldInputVerified never actually persisted the marker into the input file. The helper now reopens and asserts IsVerified. 3. Mark writers refuse unfinished syncs on both engines, and ToPebble only carries the marker alongside a finished source. The marker is now unrepresentable on an unfinished sync through the public API. 4. Sync() clears any prior verification right after binding the sync, before collection writes. A rebound, already-sealed VERIFIED sync (WithSyncID - the compactor's expansion pass, or a reused syncer) otherwise reads as verified while collection rewrites the data the marker vouched for. Pinned by TestRebindingSealedSyncClearsStaleVerification, which fails without the clear on both engines. Not changed: the c1z sanitizer intentionally does not carry the marker - sanitize rewrites record contents, so an unverified output is truthful. Co-authored-by: Cursor <cursoragent@cursor.com>
A conflict-free rebase is not a semantically current one: main's new verification-metadata functions assigned the dbUpdated flag this branch made atomic, the persisted coverage lists gained I10, and the pass now requires the active sync ID to clear prior verification before re-evaluating. Co-authored-by: Cursor <cursoragent@cursor.com>
2 participants
Persist ingest invariant verification metadata
Adds a durable verification marker to sync-run metadata recording that a sync
passed the post-collection ingestion invariants: which contract generation ran
(
ingest_invariant_generation), which invariant IDs actually executed(
ingest_invariant_coverage), and under which verdict policy(
ingest_invariant_mode:connector/compaction_merge, with_fail_fastvariants). Downstream consumers (e.g. compaction choosing a base sync) can now
distinguish "validated" from "predates validation" instead of assuming.
Semantics
verification after a successful invariant pass and persists it only after
EndSyncsucceeds — a crash anywhere before the seal leaves the artifactunverified (fail-closed). Both engines' writers refuse to mark an unfinished
sync.
is complete and an absent marker is safe, while a returned error would make
IsSyncPreservablecallers discard a finished sync.rebound sealed sync (
WithSyncID— the compactor's expansion pass, a reusedsyncer) can't read as verified while collection rewrites its data.
fail-fast); Pebble full syncs record the referential family (I5, I7, I3,
I8, I9). Partial syncs record only all-sync-type checks.
generation + required coverage against their own contract.
the post-fold expansion sync re-verifies and re-marks. The c1z sanitizer
intentionally does not carry the marker (it rewrites record contents).
sources (byte-identical data).
SyncMetaimplementations are untouched: the writer is aseparate optional interface (
IngestInvariantVerificationWriter); storeswithout it just produce unmarked (truthfully unverified) syncs.
Pre-existing bugs fixed along the way
CheckpointSync/EndSyncrebuilt the record from a partial field list;they now clone-and-mutate, preserving all fields.
SyncMetamutations skipped the store dirty bit: a standalonemetadata stamp on a reopened c1z was dropped at
Close(the envelope saveis dirty-gated).
pebbleStorenow wrapsSyncMeta()in the samedirty-marking pattern as
Grants()/FileOps()— this also covers thepre-existing
MarkSyncSupportsDiff/RecalculateStats.Testing
TestCrashBetweenInvariantPassAndSealLeavesNoMarkeraborts areal sync between the invariant pass and the seal on both engines and
asserts the unfinished sync reads unverified; the resume then seals and
marks.
TestRebindingSealedSyncClearsStaleVerificationrebinds a sealedverified sync, aborts mid-collection, and asserts the marker is gone
(fails without the clear, both engines).
SQLite column migration from pre-marker files; conversion round-trip;
fold non-inheritance (the marking helper now proves the marker persisted
into the input file — it previously didn't, making the assertion vacuous);
standalone Pebble stamp persistence (
TestPebbleStoreSyncMetaMarksDirty).fixed: post-seal error semantics, dirty-bit escape, unfinished-sync
marking, rebind staleness.