fix(simulation): store uploaded shards the way CAS does for LocalClient - #962
Open
sirahd wants to merge 3 commits into
Open
fix(simulation): store uploaded shards the way CAS does for LocalClient#962sirahd wants to merge 3 commits into
sirahd wants to merge 3 commits into
Conversation
The simulation rebuilt each uploaded shard through `MDBInMemoryShard` and stored that, while CAS re-serializes through the streaming serializer and keys the object by the hash of those bytes. Only the streaming path stamps `shard_creation_timestamp`, so identical content collapsed onto one hash forever in the simulation but not in production. GC deregisters a shard's dedup rows an epoch before condemning it, and a re-upload landing back on that same hash re-registered them, leaving rows pointing at a condemned shard — the integrity failure the high_fp stress test hits. Two consequences of storing what CAS stores. Shards written this way carry no lookup tables, so `get_file_chunk_hashes` reads its file entry out of the authoritative table like the reconstruction path already did instead of searching the shard. And they record a zero key expiry, which the shard manager's opening scan treats as long expired and prunes, so this server-side store is no longer scanned as if it were a client cache.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 81a3295. Configure here.
Re-serializing with verification off dropped each file's verification entries, so `gap_verification` came back empty from `get_file_chunk_hashes` and `upload_ranges` could not rebuild verification for stable segments. CAS trims verification too, but serves file info from its own database, which keeps it; here the stored shard is what file lookups read. `serialize` rejects the request unless every file carries verification, so ask only when they all do.
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.

The simulation rebuilt each uploaded shard through
MDBInMemoryShard, while CAS re-serializes through the streaming serializer and keys the object by the hash of those bytes. Only the streaming path stampsshard_creation_timestamp, so identical content collapsed onto a single hash forever in the simulation but not in production. That let a re-upload land back on a hash GC had already deregistered from the global dedup table and still had queued for deletion, leaving dedup rows pointing at a condemned shard —stress_test_high_fpfailed on it 7 runs out of 7.Two consequences of storing what CAS stores:
get_file_chunk_hashesreads its file entry from the authoritative table, as the reconstruction path already didTest plan: full GC stress suite against this branch with no GC-side change, all three variants green and
integrity=36ok/0FAILeach (stress_test_high_fp14/14 epochs,stress_test,stress_test_convergence), at unchanged throughput and unchanged collection rates.Note
Medium Risk
Changes simulation shard hashing, on-disk layout, and file lookup paths that GC stress tests depend on; behavior is intentional parity with CAS but affects dedup/GC edge cases in tests only.
Overview
Aligns simulation
LocalClientshard persistence with production CAS so GC stress tests match real dedup/GC behavior.Shard uploads no longer rebuild via
MDBInMemoryShard; validated input is re-serialized through the streaming path (HashedWrite+minimal_shard.serialize), keyed by the hash of those bytes (client-supplied hash ignored), written atomically via temp file + rename. Identical logical content therefore gets a new shard hash when the footer stamps a creation timestamp—fixing cases where re-upload reused a hash already deregistered/queued for deletion while dedup rows still pointed at it.Shard manager init disables directory scan on open (
scan: false) because CAS-form shards use zero key expiry; scanning would incorrectly prune them as if this were an expiring client cache.get_file_chunk_hashesresolves file metadata fromFILE_TO_SHARD_TABLE(get_file_info_from_table) instead of shard-manager reconstruction lookup, consistent with shards stored without embedded lookup tables.Reviewed by Cursor Bugbot for commit 5cec0e0. Bugbot is set up for automated code reviews on this repo. Configure here.