Repository navigation
fix(core): compare storage and content checksums like with like; record the materialized object's storage checksum - #1660
Merged
Conversation
…rd the materialized object's storage checksum
Every note the app saves is indexed in the request, then materialized to
storage. Two defects made the storage notification for that write re-read and
fully re-index the note, and in cloud mark it superseded:
1. Materialization never recorded the written object's storage checksum.
RuntimeWrittenFileState now carries storage_checksum (the PUT's ETag in
cloud, the content sha256 locally), and the publisher records it as
entity.checksum in the same guarded update that clears sync_checksum. The
existing index gate then recognizes the materialized object as current.
2. One helper mixed two kinds of checksum. storage_object_checksum_for_index_match
returned bm-file-checksum (a content sha256) as if it were a storage checksum,
and the planners compared it with entity.checksum / the indexed checksum. Since
cloud indexing records S3 ETags (#2350 in cloud) those never match, so every
app-written note indexed through a storage notification was marked
content_superseded: its embeddings job was skipped and its provenance dropped.
Each check now reads one kind:
- supersession: the object's storage checksum vs the indexed storage checksum
(a different object means a newer write replaced it, whoever wrote it);
- provenance trust (#1589): bm-file-checksum vs the content checksum of the
markdown indexed. FileIndexResult carries content_checksum, and
CurrentMaterializedNoteEntity carries storage_checksum (entity.checksum) and
content_checksum (note_content.file_checksum).
The mixing helper, its source enum and the unused diagnostic plan fields are
removed.
Tests: planner tests rewritten in the cloud shape (ETag storage, sha256 content),
including the regression that an app-written note is neither superseded nor
stripped of provenance; a new integration test proves a published materialization
is recognized by the index gate (fails without the publisher change).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T8wjd6HrtSA2LN9ssC4NzF
Signed-off-by: phernandez <paul@basicmachines.co>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
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
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.
Core prerequisites C1 to C3 for basic-memory-cloud's
docs/MATERIALIZATION_WEBHOOK_PLAN.md.Problem
Every note the app saves is indexed in the request, then materialized to storage. When storage notifies us about that write:
IndexedChecksums.recognizes, which compares the object's storage checksum withentity.checksum) cannot recognize it. In production that is ~23k re-reads and full re-indexes a day.storage_object_checksum_for_index_matchreturnedbm-file-checksum(a content sha256) as if it were a storage checksum, and the planners compared it with the indexed checksum. Cloud indexing records S3 ETags, so those never match. Every app-written note indexed through a notification was markedcontent_superseded, which skipped its embedding job and dropped its provenance.Change
C1/C2: record the materialized object's storage checksum.
RuntimeWrittenFileStategainsstorage_checksum: the PUT's ETag in cloud, the content sha256 on a local filesystem.entity.checksum, in the same guarded update that clearssync_checksum.C3: each check compares one kind of checksum.
bm-file-checksumagainst the content checksum of the markdown indexed.FileIndexResultgainscontent_checksum.CurrentMaterializedNoteEntitycarriesstorage_checksum(entity.checksum) andcontent_checksum(note_content.file_checksum), loaded alongside the entity.file_checksum_from_object_metadatais now typed as a content checksum.Behavior change to review
Supersession now fires for any writer whose newer object replaced the file mid-job. Before, it fired only when own-stack
bm-*metadata was present, because an ETag never equalled a sha256. A superseded result skips embeddings and withholds the content checksum and version from live updates; the newer write's own notification job indexes the current content. Locally, storage checksums are sha256s, so local results are unchanged.Tests
bm-*metadata on different bytes is not trusted;test-int/test_materialized_note_index_gate.pypublishes a materialization against a real database and asserts the index gate recognizes the written object's ETag. Confirmed it fails with the publisher change removed.just fast-checkpasses, and 964 tests intests/indexing,tests/index,tests/cloudandtests/test_runtime.pypass.Follow-up in cloud
The cloud materialization writer must return the written ETag as
storage_checksum. That's a required field, so it goes in the same cloud PR that bumps the pin.🤖 Generated with Claude Code
https://claude.ai/code/session_01T8wjd6HrtSA2LN9ssC4NzF