Repository navigation
feat(cas_types): report the stored shard hash from a shard upload - #1003
Conversation
54f91f7 to
fb1b5f6
Compare
fb1b5f6 to
cc5d7d7
Compare
| } | ||
|
|
||
| Ok(()) | ||
| Ok(shard_hash) |
There was a problem hiding this comment.
nit: You mentioned GC uses its own client, so you probably only need the interface and simulations changes. So let's not change this function or upload_shard_with_version_override
There was a problem hiding this comment.
Sounds good. I went ahead with your suggested change, with a caveat that it results in SimulationControlClient unable to reuse RemoteClient upload shard implementation and having to re-implement the upload. It's not a big deal, just something to be aware of.
There was a problem hiding this comment.
Ah.. sorry I didn't realize that SimulationControlClient depends on RemoteClient. Then for your convenience I think it's Ok to have RemoteClient actually returning the value you need
| let forced_version = self.ctx.config.client.shard_api_version; | ||
| self.upload_shard_with_version_override(shard_data, upload_permit, forced_version, progress_callback) | ||
| .await | ||
| } |
There was a problem hiding this comment.
Instead, just Ok(None) here
ba7af41 to
13959f8
Compare
The server re-serializes an uploaded shard and keys the object on the result, so the uploader's own hash is a prediction, not a fact. Return the stored hash so a caller that records it can use what was actually written rather than inferring it from bytes it hopes the server reproduced. `UploadShardResponse::shard_hash` is optional for servers predating the field, and `Client::upload_shard` returns `Option<MerkleHash>` rather than inventing one: the V2 NDJSON stream carries no hash and a dry run stores nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
13959f8 to
5ea99da
Compare
2 participants
The server re-serializes an uploaded shard and keys the object on the result, so the uploader's own hash is a prediction rather than a fact. This returns the stored hash so a caller that records it can use what was actually written.
UploadShardResponse::shard_hashisOption, omitted by servers predating it.Client::upload_shardreturnsOption<MerkleHash>rather than inventing one: a dry run stores nothing, and the V2 NDJSON stream carries no hash./v1/shardsreports the hashLocalClientstored under, so downstream tests exercise the real path.Not a wire break:
UploadShardResponsehas nodeny_unknown_fields, so a client compiled before this field keeps parsing responses that carry it — pinned bytest_upload_shard_response_is_readable_without_the_new_field.🤖 Generated with Claude Code
Note
Medium Risk
Changes a core
Clienttrait signature and upload response parsing; callers must handleOptionand V2 having no hash, but wire format remains backward compatible.Overview
Shard uploads now surface the authoritative stored hash from the server instead of treating the client’s pre-upload hash as ground truth.
Wire / API:
UploadShardResponsegains an optionalshard_hash(HexMerkleHash), omitted for older servers and when absent. Serde tests lock backward/forward compatibility.Client contract:
Client::upload_shardreturnsResult<Option<MerkleHash>>instead ofResult<()>. V1/v1/shardsJSON suppliesSome(hash); V2 NDJSON and dry-run returnNone. V2→V1 fallback forwards the V1 hash.Simulations:
LocalClientreturnsSomeafter CAS-style re-serialize;MemoryClientreturnsNone(merged shard). Local/v1/shardshandler echoesshard_hashin the JSON response.Reviewed by Cursor Bugbot for commit 5ea99da. Bugbot is set up for automated code reviews on this repo. Configure here.