[FIX] Derive the S3 document key suffix from the document so a retry overwrites - #518
Open
noel-improv wants to merge 4 commits into
Open
[FIX] Derive the S3 document key suffix from the document so a retry overwrites#518noel-improv wants to merge 4 commits into
noel-improv wants to merge 4 commits into
Conversation
A source id is md5(text)[:8] plus md5(metadata_str)[:4], so 48 bits, and 32 when a document carries no metadata because the second component is then constant. Distinct documents collide at scale: 127 pairs at 1M documents, 11,762 at 10M. The text component's width is now a setting, read from SOURCE_ID_HASH_LENGTH and defaulting to 8, which is what existing graphs were written with. Widening it changes every source id and chunk id, so a graph written at one width cannot be read at another. Chunk text is unaffected, so a re-ingest reproduces the same chunks under new ids. Plumbed from GraphRAGConfig at both IdGenerator call sites, so the setting reaches the ids a run writes.
…dGenerator IdGenerator took the width as a plain default, so the setting only applied at the one call site that passed it and IdGenerator() ignored the config. It now coalesces to GraphRAGConfig, as include_classification_in_entity_id does. A width outside the digest is rejected where it is set as well as where it is used; the config setter accepted 0 and 64. Drop the width from BuildPipeline. create_source_id and create_chunk_id are called only from IdRewriter, which the build path never constructs. Tests reset the env var and the cached config, so a value in either place no longer changes ids for the tests that follow. The collision test patched the hash to a constant, which made it pass at any width; it now uses two texts that genuinely collide at the width under test. Document the setting.
The suffix is uuid4, so the key is different on every attempt and a retry writes a second object beside the first. A restart cannot tell a re-stage from a duplicate. Behind a flag the suffix is a digest of the document's own node ids, so a retry overwrites. Keying on source_id alone would not do: _extract_auto_tuned emits one source as several SourceDocuments when it exceeds the round capacity, and those would then overwrite each other. The node ids differ per round, so the rounds stay apart. Off by default, so existing collections keep today's layout.
Three corrections to the deterministic key. The suffix hashed every node on the SourceDocument, but the body holds only those without an INDEX_KEY, so the key was not a function of the object's contents. Both now read the same list. Joining node ids with no separator let ['ab', 'c'] and ['a', 'bc'] hash alike. They are joined on a null byte instead, matching use_chunk_id_delimiter. A source prefix collects an object per run, so a node id can arrive twice when a later run packs the chunks differently. The reader now keeps the first copy of an id rather than returning it twice. S3ChunkUploader loses the flag: it keys on node_id, which carries no random component, so it had nothing to switch.
noel-improv
force-pushed
the
fix/deterministic-document-key
branch
from
September 4, 2026 00:01
6a50c00 to
70e2625
Compare
noel-improv
marked this pull request as ready for review
September 4, 2026 16:01
oussamahansal
approved these changes
Sep 8, 2026
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
3 participants
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.
Description
Stacked on #517. That PR is the first commit here and is not part of this review. Until it merges, the diff below shows both; the change under review is
feat(lexical-graph): derive the S3 document key suffix from the document.S3DocUploader._upload_docbuilds the key as{source_id}-{uuid4:5}.jsonl, so the key is different on every attempt. A retry writes a second object beside the first instead of replacing it, and a restart cannot tell a re-stage from a duplicate.Behind a flag the suffix becomes a digest of the document's own node ids. A retry then produces the same key and overwrites.
Changes
deterministic_document_keyonS3DocUploader,S3ChunkUploaderandS3BasedDocs, defaulting toFalse. With it off, keys match today's layout exactly.get_hash(sorted node ids)[:5]when the flag is on,uuid4().hex[:5]when it is off.Keying on
source_idalone would not work._extract_auto_tunedemits one source as severalSourceDocuments when a document exceeds the round capacity, and those would overwrite each other. Node ids differ per round, so the rounds stay in separate objects while a retry of one round still overwrites. Both properties come from the same expression and both are covered.Problem
The random suffix is also what currently keeps two colliding documents in separate objects, so dropping it needs the wider key from #517. Sizing that collision is what #515 measured: 127 colliding pairs at 1M documents on the 32 bits that discriminate when metadata is absent.
Related issue (if any): #325. A restart cannot resume against keys that change on every attempt.
Testing
pytest) — 2,094 intests/unitNine tests covering both flag states, retry stability, round separation, and independence from the order chunks arrive in.
One pre-existing failure is unrelated and reproduces on a clean tree:
test_integ_dependency_compatibility.pyerrors withNo module named pipin this venv.Checklist
The flag is not yet reachable from configuration, so today it is set by constructing
S3BasedDocsdirectly. Wiring it to a setting belongs with the rest of the restart work rather than here.