feat: read/write data integrity check (block_hash + integrity config) - #212
feat: read/write data integrity check (block_hash + integrity config)#212charpty wants to merge 34 commits into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad72c429cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // 数据校验码，由 client 在写入完成后通过 FinishWriteCacheRequest.locations 上报， | ||
| // 在 GetCacheLocation / GetCacheMeta 返回时供读端比对。0 表示未设置 (老数据 / 老 client)， | ||
| // 读端遇到 0 必须跳过校验。同一 block 的多副本必须写入相同 hash。 | ||
| int64 block_hash = 4; |
There was a problem hiding this comment.
When enable_meta_checksum is used, this field has to round-trip through GetCacheLocation/GetCacheMeta so callers can pass non-zero expected_hashes into LoadKvCaches, but the response path still drops it: CacheLocationViewToProto only sets type/spec_size/location_specs, CacheLocationView has no hash accessor, and the client GenLocations collapses response.locations() to URI-only Locations. In that configuration, writes can persist a hash but read clients receive no expected hashes, so corrupted reads are silently treated like old data and never verified.
Useful? React with 👍 / 👎.
| VineyardStorageSpec vineyard = 10; | ||
| } | ||
| bool check_storage_available_when_open = 8; | ||
| DataIntegrityConfig integrity = 11; // 数据校验配置，缺省视为全部关闭 |
There was a problem hiding this comment.
The admin AddStorage/UpdateStorage path builds StorageConfig via ProtoConvert::StorageFromProto, but that converter never reads request->storage().integrity(), and StorageConfigToProto also omits it for ListStorage. As a result, users configuring enable_meta_checksum or the rejected inline-header options through admin_service.proto have those settings silently dropped, so checksum never turns on and invalid inline-header configs are not rejected on this API path.
Useful? React with 👍 / 👎.
| if (static_cast<int>(block_hashes.size()) != proto_locations->size()) { | ||
| KVCM_LOG_ERROR("block_hashes size [%zu] mismatches locations size [%d]; skip hash propagation", | ||
| block_hashes.size(), | ||
| proto_locations->size()); |
There was a problem hiding this comment.
When block_hashes is non-empty but locations is empty or lacks failed-block placeholders, this branch only logs the mismatch and still sends the RPC without any hashes. That matches existing FinishWrite(..., {}) call patterns, so a caller can compute valid hashes and pass them here but the server will mark the write serving with block_hash=0; return an error or synthesize placeholder CacheLocations instead of silently disabling integrity.
Useful? React with 👍 / 👎.
| static_cast<unsigned long>(expected), | ||
| static_cast<unsigned long>(actual[i]), | ||
| i < uri_str_vec.size() ? uri_str_vec[i].c_str() : "<oob>"); | ||
| return ER_CHECKSUM_MISMATCH; |
There was a problem hiding this comment.
The mismatch path only logs and returns ER_CHECKSUM_MISMATCH; the newly added ChecksumMismatchEvent has no call site (rg ChecksumMismatchEvent only finds the class definition). If production monitoring consumes the event stream rather than client logs, detected data corruption will not generate the documented integrity event.
Useful? React with 👍 / 👎.
| if (out_block_hashes != nullptr || need_print) { | ||
| auto handle = sdk_buffer_check_pool_->GetCell(); | ||
| block_hashs = SdkBufferCheckUtil::GetBlocksHash(block_buffers, |
There was a problem hiding this comment.
When the checksum pool is enabled and out_block_hashes is requested, this hashes block_buffers before sdk_wrapper_->Put() runs its existing Valid() checks. SdkBufferCheckUtil::GetBlocksHash dereferences block_buffers.front(), so an empty/invalid SaveKvCaches call that used to return ER_INVALID_PARAMS can now crash or produce hashes for an operation that later fails; validate the URI/buffer inputs before calling GetBlocksHash.
Useful? React with 👍 / 👎.
| auto handle = sdk_buffer_check_pool_->GetCell(); | ||
| auto actual = SdkBufferCheckUtil::GetBlocksHash( | ||
| block_buffers, handle->d_iovs, handle->d_crcs, handle->h_iovs, max_check_iov_num_, handle->gpu_stream); |
There was a problem hiding this comment.
For reads that use Iov::ignore (the SDKs support this for partial reads), this verification path hashes the original block_buffers, but SdkBufferCheckUtil hashes every IOV and does not honor the ignore flag while the read leaves ignored ranges untouched. With expected_hashes supplied, those stale ignored ranges can make otherwise valid partial reads return ER_CHECKSUM_MISMATCH; either reject checksum verification for ignored IOVs or compute the hash over exactly the transferred slices.
Useful? React with 👍 / 👎.
| if (integrity.enable_meta_checksum()) { | ||
| any_meta_checksum_enabled = true; |
There was a problem hiding this comment.
A storage config with enable_meta_checksum=true and an unrecognized algo parses as CA_UNSPECIFIED, and StorageConfig::ValidateRequiredFields would reject it, but this client-side Init validator only turns on the checksum pool. Direct TransferClient/ManagerClient initialization with such storage_configs therefore silently uses the CRC32 path for a config that should be invalid; call ValidateRequiredFields or explicitly check integrity.algo() here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2e98a42b06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!any_compared || expected_xor == actual_xor) { | ||
| return result; // all match (or nothing to check) |
There was a problem hiding this comment.
When strict mode is off (the default used by LoadKvCaches), this returns success solely because the aggregate XORs match. XOR is order-insensitive, so a batch whose two blocks are swapped, e.g. expected [A, B] but actual [B, A], or any paired cancellation, skips the per-block fallback and reports ER_OK even though the affected buffers are wrong; the new unit test constructs exactly this fast-path miss. Please compare per block before accepting the batch, or use an order-sensitive aggregate that cannot hide block swaps.
Useful? React with 👍 / 👎.
Bundle the seven issues codex flagged on PR #212 into a single follow-up commit so each fix is reviewable next to its motivation. P1 fixes (Scheme A read path was actually broken): * CacheLocation.checksum now round-trips through the read RPCs. CacheLocationView gained a checksum() accessor and CacheLocationViewToProto fills the new proto field, so GetCacheLocation / GetCacheMeta / MatchLocation responses carry the stored checksum back to the client. Stub::GetCacheMeta and Stub::GetCacheLocation gained an optional std::vector<int64_t>* out_checksums; GrpcStub::GenLocations writes the checksum into it. MetaClient::MatchLocation, MetaClient::MatchMeta, and the ManagerClient mirror APIs propagate the same optional parameter. Without this the freshly-persisted checksum never reached the reader and LoadKvCaches's verification path could not be triggered. * GrpcStub::FinishWriteCache now returns ER_INVALID_PARAMS when the supplied checksums vector length does not match the protobuf locations count, instead of warning and silently sending a checksumless RPC that would commit CacheLocation.checksum = 0 while the caller believed the value was reported. P2 fixes: * ProtoConvert::StorageConfigToProto / StorageFromProto round-trip the integrity config so AddStorage / UpdateStorage / ListStorage on the admin API no longer silently drop enable_meta_checksum and so the Validate rejection of inline_header keeps working on this path. A small helper handles the admin::ChecksumAlgo <-> C++ ChecksumAlgo conversion with an explicit switch. * TransferClientImpl::ValidateStorageConfigsForIntegrity now invokes DataIntegrityConfig::ValidateRequiredFields after the bespoke inline_header checks. This catches enable_meta_checksum=true with algo=CA_UNSPECIFIED and any other future invariant the canonical validator adds, so client init no longer silently falls back to CRC32 for configs the server would reject. * SaveKvCaches now skips the GPU checksum compute when block_buffers is empty or the first block has no iovs. SdkBufferCheckUtil::GetBlocksHash dereferences block_buffers.front(), so an empty / malformed input that previously returned ER_INVALID_PARAMS could crash the hashing kernel before sdk_wrapper_->Put() ran its own input validation. * LoadKvCaches now treats blocks containing an iov with ignore=true (partial reads -- the SDK leaves ignored ranges untouched) as unverifiable: the matching expected_checksums entry is downgraded to the 0 sentinel so the verifier skips it. Empty-iovs blocks get the same treatment. Without this a legitimate partial read would always raise ER_CHECKSUM_MISMATCH. * Mismatch logging is now structured ("ChecksumMismatchEvent {block_index=..., expected_checksum=..., actual_checksum=..., storage_uri=..., trace_id=...}") mirroring the ChecksumMismatchEvent field set, so a log scraper can build the same observability surface until the client SDK gains a real EventManager hook. Full event publishing remains a tracked follow-up in the design doc. All UTs pass under ASAN debug on the external source mode (38/38). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 00eecc9628
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| inline DataStorageType type() const { return cache_location_.type(); } | ||
| inline int32_t spec_size() const { return cache_location_.spec_size(); } | ||
| inline const LocationSpecViewVec &location_specs() const { return location_specs_view_; } | ||
| inline int64_t checksum() const { return cache_location_.checksum(); } |
There was a problem hiding this comment.
Fresh evidence: after adding this accessor, the normal GetCacheLocation paths still pass synthetic CacheLocation objects from SelectAndMergeForMatch/BatchGetBestLocationByBackend; those merged objects are constructed with type/specs only and never copy winner->checksum(). In QT_BATCH_GET, prefix, and reverse-window reads this accessor therefore returns the default 0, CacheLocationViewToProto emits the sentinel, and read clients skip verification even though FinishWrite persisted a checksum; set the merged location checksum from the selected location before returning it.
Useful? React with 👍 / 👎.
| auto handle = sdk_buffer_check_pool_->GetCell(); | ||
| auto actual = SdkBufferCheckUtil::GetBlocksHash( | ||
| block_buffers, handle->d_iovs, handle->d_crcs, handle->h_iovs, max_check_iov_num_, handle->gpu_stream); |
There was a problem hiding this comment.
When expected_checksums is non-empty but every entry is 0 (legacy data) or every block is made unverifiable by empty/ignored IOVs, this path still hashes block_buffers before VerifyBatchChecksums can skip all entries. In CUDA/MUSA builds with the checksum pool enabled, an empty/no-IOV block reaches SdkBufferCheckUtil::GetBlocksHash, which uses the first block's IOV count and can assert/divide by zero, so an all-sentinel verification request can crash instead of being a no-op; return early when effective_expected has no non-zero checksum left.
Useful? React with 👍 / 👎.
| } | ||
| // integrity 字段是后加的，老配置不带该字段时保留默认值 (全部关闭)。 | ||
| if (rapid_value.HasMember("integrity") && rapid_value["integrity"].IsObject()) { | ||
| integrity_.FromRapidValue(rapid_value["integrity"]); |
There was a problem hiding this comment.
When an integrity object is present but one of its fields has the wrong type, for example "enable_meta_checksum": "true", DataIntegrityConfig::FromRapidValue returns false but this call ignores that result and still accepts the storage config with default integrity settings. That silently disables checksum or inline-header validation for malformed configs instead of rejecting them, so propagate the nested parser failure here.
Useful? React with 👍 / 👎.
| bool need_print = (trace_info == nullptr) ? true : trace_info->need_print; | ||
| if (out_checksums != nullptr || need_print) { | ||
| auto handle = sdk_buffer_check_pool_->GetCell(); | ||
| block_checksums = SdkBufferCheckUtil::GetBlocksHash(block_buffers, |
There was a problem hiding this comment.
When a checksum-enabled write contains more IOVs than KVCM_SDK_MAX_CHECK_IOV_NUM, SdkBufferCheckUtil::GetBlocksHash stops at the pool limit and returns hashes only for the prefix, but SaveKvCaches still returns ER_OK and hands that shorter vector to the caller. The subsequent FinishWrite then fails the checksum/location length check after the data has already been written, so large valid batches cannot be committed; reject or chunk oversized checksum inputs before returning success.
Useful? React with 👍 / 👎.
| if (storage_spec_ && !storage_spec_->ValidateRequiredFields(local_invalid_fields)) { | ||
| valid = false; | ||
| } | ||
| if (!integrity_.ValidateRequiredFields(local_invalid_fields)) { |
There was a problem hiding this comment.
This call only protects paths that explicitly run StorageConfig::ValidateRequiredFields; the startup/recovery paths I checked parse JSON and register storage directly (StartupConfigLoader::Load -> RegistryManager::AddStorage, and RegistryManager::RecoverStorageUnsafe -> DataStorageManager::RegisterStorage) without invoking it. A startup or persisted config with enable_inline_header=true or enable_meta_checksum plus an unsupported algo is therefore accepted into the registry instead of being rejected at startup, and workers later fail Init or run with a config the server said was valid; move this validation into the registry add/recover path or call it before registration.
Useful? React with 👍 / 👎.
![qoderai[bot]](https://avatars.githubusercontent.com/in/1396158?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Thanks for the detailed design doc and the careful backwards-compatibility work on Scheme A. The additive API changes and sentinel handling look solid.
Key Risks & Issues
-
SaveKvCaches output on Put failure:
out_checksumsis filled beforesdk_wrapper_->Put()succeeds, so a failed write can still produce checksums that the caller might persist. A caller that forwards those checksums toFinishWritewould store a hash for a block that does not exist on disk, which defeats the integrity check. See the inline comment onkv_cache_manager/client/src/transfer_client_impl.cc. -
Mismatch log uses wrong block id: The checksum mismatch log uses
trace_info->block_ids.front()for every faulty block. SinceTransferTraceInfo::block_idsis documented as parallel toblock_buffers, this prints the wrong identifier for any mismatch after the first one. See the inline comment onkv_cache_manager/client/src/transfer_client_impl.cc. -
I also reviewed the earlier automated feedback and did not duplicate those points; the merged-location checksum propagation, admin integrity conversion, registry validation, and event publication items remain worth addressing separately.
Verification Advice
- Add a unit test where
sdk_wrapper_->Put()returns an error and verify thatout_checksumsremains empty/cleared. - Add a multi-block mismatch test with
TransferTraceInfopopulated and assert the logged block id matches the mismatched index.
Thoughts & Suggestions
- The fast XOR aggregate plus strict-mode fallback is a reasonable tradeoff given the documented ~2^-64 probability; making it opt-in via
KVCM_CHECKSUM_STRICT_MODEis the right call. - Consider keeping
CacheLocationToProto/CacheLocationFromProtosymmetric withCacheLocationViewToProtoso future callers don't accidentally drop checksums.
🤖 Generated by Qoder
| } | ||
| PrintBlockHashAndUri("put_", uri_str_vec, block_hashs, trace_info); | ||
| if (out_checksums != nullptr && block_checksums_computed) { | ||
| *out_checksums = block_checksums; |
There was a problem hiding this comment.
We assign *out_checksums before sdk_wrapper_->Put() has run, so if Put() fails we still hand the caller checksums for data that was never persisted. A caller that then forwards those checksums to FinishWrite will store a hash for a block that does not exist on disk, which defeats the integrity check. Consider moving the assignment so it only happens after Put() returns ER_OK.
🤖 Generated by Qoder
| static_cast<unsigned long>(actual[idx]), | ||
| idx < uri_str_vec.size() ? uri_str_vec[idx].c_str() : "<oob>", | ||
| (trace_info != nullptr && !trace_info->block_ids.empty()) | ||
| ? trace_info->block_ids.front().c_str() |
There was a problem hiding this comment.
The per-block mismatch log uses trace_info->block_ids.front() for every faulty index. Since TransferTraceInfo::block_ids is documented as parallel to block_buffers, this prints the wrong block identifier for any mismatch after the first one. Should this use trace_info->block_ids[idx] (with a bounds check) instead?
🤖 Generated by Qoder
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f8abfe9b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // integrity 字段是后加的，老配置不带该字段时保留默认值 (全部关闭)。 | ||
| // 存在但字段类型错乱 (例如 "enable_meta_checksum": "true" 用了字符串) | ||
| // 必须让整个 StorageConfig 解析失败，否则会静默降级为「全关」。 | ||
| if (rapid_value.HasMember("integrity") && rapid_value["integrity"].IsObject()) { |
There was a problem hiding this comment.
When a storage JSON contains an integrity member that is not an object, such as "integrity": "bad", this condition is false and parsing succeeds with the default all-disabled integrity settings. That silently drops checksum enablement and inline-header validation for a malformed config; if the member exists but is not an object, StorageConfig::FromRapidValue should fail instead of treating it like an absent legacy field.
Useful? React with 👍 / 👎.
| auto handle = sdk_buffer_check_pool_->GetCell(); | ||
| auto actual = SdkBufferCheckUtil::GetBlocksHash( | ||
| block_buffers, handle->d_iovs, handle->d_crcs, handle->h_iovs, max_check_iov_num_, handle->gpu_stream); |
There was a problem hiding this comment.
When a read batch has at least one real expected checksum and another block was downgraded to the zero sentinel because it has empty or ignored IOVs, has_any_expected remains true and this still hashes the original block_buffers. SdkBufferCheckUtil::GetBlocksHash uses the first block's IOV count and asserts that every block has the same nonzero count, so a mixed full/partial read can assert or fail before the sentinel entry is skipped; hash a compacted list of verifiable blocks or reject verification for such mixed shapes.
Useful? React with 👍 / 👎.
| block_checksums = SdkBufferCheckUtil::GetBlocksHash(block_buffers, | ||
| handle->d_iovs, | ||
| handle->d_crcs, | ||
| handle->h_iovs, | ||
| max_check_iov_num_, | ||
| handle->gpu_stream); |
There was a problem hiding this comment.
With meta checksums enabled, checksum_input_usable only checks the first block before calling GetBlocksHash on the entire batch. If a later block has no IOVs or a different IOV count (for example from mixed location-spec groups), the checksum helper hits its all-blocks-same assertion before sdk_wrapper_->Put() can reject or handle the input, so a checksum-enabled write can crash instead of returning an error; validate all blocks or hash per block safely before this call.
Useful? React with 👍 / 👎.
| for (const auto &[inst_id, hosts] : instance_nodes_) { | ||
| auto it = hosts.find(host_ip_port); | ||
| if (it != hosts.end() && it->second && it->second->available.load(std::memory_order_relaxed)) { |
There was a problem hiding this comment.
When the same host_ip_port is registered for another instance but is down or unregistered for the instance currently being matched, this scan still returns true because it accepts the host from any instance_nodes_ bucket. GetCheckLocDataExistFunc is invoked for a specific instance_id, so treating another instance's node as proof that this instance's V6D location exists leaves stale locations alive and can route reads to data that should have been isolated/pruned for the current instance.
Useful? React with 👍 / 👎.
| this->CleanupHostLocations(instance_id, host_ip_port, gen_at_trigger, requested_type); | ||
| }); | ||
| vineyard_backend->UnregisterNode(host_ip_port); | ||
| event_backend->UnregisterNode(instance_id, host_ip_port); |
There was a problem hiding this comment.
If a stale EVENT_HOST_DOWN races with a fast node restart, RegisterNode can increment the generation and refresh the node after gen_at_trigger is captured but before this unconditional unregister runs. In that case the cleanup task will correctly skip as stale, but this line still erases the freshly re-registered node, causing subsequent heartbeats to get NODE_NOT_REGISTERED; re-check the generation before unregistering as the liveness cleanup path does.
Useful? React with 👍 / 👎.
| int64 heartbeat_timeout_ms = 1; | ||
| int64 cleanup_grace_ms = 2; | ||
| int64 liveness_check_interval_ms = 3; |
There was a problem hiding this comment.
Changing VineyardStorageSpec to reuse tag 1 for heartbeat_timeout_ms and to shift the existing timeout fields from tags 2/3/4 to 1/2/3 breaks protobuf compatibility for rolling upgrades and older admin clients. An old client that sends heartbeat_timeout_ms on tag 2 is now decoded as cleanup_grace_ms, cleanup_grace_ms becomes liveness_check_interval_ms, and old tag 4 is ignored, so custom liveness timing is silently corrupted; keep the old tag numbers and reserve/remove only cluster_name.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3369178a66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| is_check_buffer_ = EnvUtil::GetEnv("KVCM_SDK_CHECK", false); | ||
| if (is_check_buffer_) { | ||
| meta_checksum_enabled_ = any_meta_checksum_enabled; | ||
| if (is_check_buffer_ || meta_checksum_enabled_) { |
There was a problem hiding this comment.
When a storage config enables meta_checksum, this branch now runs even without KVCM_SDK_CHECK; if SdkBufferCheckPool::Init fails, the function returns ER_INIT_CHECK_BUFFER_ERROR without clearing client_config_/sdk_wrapper_, so a retry hits the early client_config_ != nullptr path and reports ER_OK even though checksum support was never initialized. Clear the partial init state before returning, consistent with the other init failure paths.
Useful? React with 👍 / 👎.
| if (!ig || ig->event_reporting_storage_candidates().empty()) { | ||
| return nullptr; |
There was a problem hiding this comment.
For instance groups persisted before event_reporting_storage_candidates was added, deserialization leaves this vector empty, and this new early return makes ReportEvent fail with INSTANCE_NOT_EXIST instead of falling back to the previous per-instance Vineyard backend lookup (v6d_<instance_id>). In an upgrade where the registry data has not been rewritten yet, V6D node/register/block events stop being accepted, so add a legacy fallback or migration path before returning null here.
Useful? React with 👍 / 👎.
| if (!storage_config.ValidateRequiredFields(invalid_fields)) { | ||
| PREFIX_LOG_S(WARN, "reject add storage: invalid config, fields[%s]", invalid_fields.c_str()); | ||
| return EC_BADARGS; |
There was a problem hiding this comment.
This new rejection path also runs from UpdateStorage, but that function removes the current storage entry before calling AddStorage; if an operator updates an existing storage with enable_inline_header=true or enable_meta_checksum plus an unspecified algo, the update now deletes the working backend/registry entry and then fails here with EC_BADARGS. Validate the replacement config before RemoveStorage or roll back the old entry on this failure path.
Useful? React with 👍 / 👎.
|
@codex review |
…ead/write integrity check 为后续读写链路数据校验功能预留 proto 字段（任务 82620492）： - 新增 ChecksumAlgo 枚举与 DataIntegrityConfig 子消息（meta / admin / kv_meta 三份 proto 各自定义一份以保持 package 独立）。 - StorageConfig 加 integrity 字段（field 11），缺省视为关闭。 - CacheLocation 加 block_hash 字段（meta / admin），0 表示未设置 - 老 data / 老 client 走原有路径。FinishWriteCacheRequest.locations 字段 已预留，复用即可承载 hash 上报。 本 commit 不动任何业务代码，仅扩展 wire format。enable_meta_checksum (方案 A) 与 enable_inline_header (方案 B 留位) 的 C++ 持久化、Validate 拒绝防线、 client/server 校验闭环在后续 commit 中加入。 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ross layers 为数据校验功能 (任务 82620492) 加入新错误码，让 client 能区分 checksum 错误 与一般 internal error： - 四份 proto (meta / admin / kv_meta / debug) 的 ErrorCode enum 同步加 CHECKSUM_MISMATCH = 21、INLINE_HEADER_INVALID = 22，保持四份值一致避免 未来撞值。 - common/error_code.h 加 EC_CHECKSUM_MISMATCH = 20、EC_INLINE_HEADER_INVALID = 21，ToPbError 模板补齐 mapping。 - client/include/common.h 加 ER_CHECKSUM_MISMATCH = 118、 ER_INLINE_HEADER_INVALID = 119，client/internal/stub/grpc_stub.cc 的 ToClientError 补齐 pb -> client 映射。 行为完全向后兼容：没有任何代码会产生这两个错误码，本 commit 仅扩展枚举值。 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…t inline_header
为任务 82620492 加入 C++ 侧 DataIntegrityConfig：
- 新增 ChecksumAlgo enum 与 ToString/ToChecksumAlgo 转换函数，C++ enum 与
proto 同名定义对齐。
- 新增 DataIntegrityConfig Jsonizable 类，承载 enable_meta_checksum /
enable_inline_header / inline_header_version / algo 四字段；
ValidateRequiredFields 内置三条防线：
(1) enable_inline_header=true 一律拒绝 (方案 B 接口预留，本期未实现)
(2) inline_header_version != 0 但开关没开 = 配置矛盾
(3) enable_meta_checksum=true 时 algo 必须显式设置
- StorageConfig 加 integrity 成员 + getter/setter，FromRapidValue 用
HasMember 检查 "integrity" 字段保证老配置 (无该字段) 反序列化向后兼容；
ValidateRequiredFields 把 DataIntegrityConfig 的拒绝结果合并到自身。
- storage_config_test 增加 5 个 case 覆盖默认值、JSON round-trip、
inline_header 拒绝、version-without-header 拒绝、老 JSON 兼容、
StorageConfig 透传拒绝；老 NFS JSON literal 断言同步更新含 integrity。
行为约束：因为 DataIntegrityConfig::ToRapidWriter 始终输出该字段，所有
依赖 ToJsonString 字面 round-trip 的下游使用方需要意识到 JSON schema
扩展 (FromJsonString 侧已兼容缺失字段)。
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
为任务 82620492 在 CacheLocation 内存与 JSON 模型加入 block_hash： - CacheLocation 新增 int64_t block_hash_ (默认 0) + 访问器；ToRapidWriter 始终输出 block_hash；FromRapidValue 用 KVCM_JSON_GET_DEFAULT_MACRO 容忍 老 meta 存量数据 (无该字段时反序列化为 0)。 - 0 是 sentinel "未设置"，读端必须显式跳过校验；这样老 client 不上报 hash 时整条链路无感降级。 - 新增 cache_location_test 单测 4 个 case：默认值、JSON round-trip、 legacy JSON 无字段、负数 hash 保留。 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ishWriteCache 为任务 82620492 把 client 上报的 block_hash 落到 CacheLocation： - MetaSearcher::LocationUpdateTask 加 int64_t block_hash (默认 0)， BatchUpdateLocationStatus 的 ReadModifyWriteLocation modifier 里在 task.block_hash != 0 时调用 CacheLocation::set_block_hash；== 0 时 保留 CacheLocation 已有 hash，避免清零。 - CacheManager::FinishWriteCache 加 const std::vector<int64_t> & block_hashes 默认参数；非空时必须与 location_info.keys 等长 (mask 前的 完整 keys，不是 mask 后的成功子集)；遍历 success_block_mask 时按下标 对齐取 hash 装到 LocationUpdateTask；空 vector 视为 client 不上报， task.block_hash 走默认 0。长度不匹配返回 EC_BADARGS。 - write-timeout 回调路径 (cache_manager.cc:703) 仍然只删 meta，没必要 改签名，默认空 hash 不影响。 - 新增 meta_searcher_test 两个 case：PersistsBlockHash 验证非零 hash 落 到 CacheLocation；ZeroHashPreservesExisting 验证 0 不覆盖已有 hash。 老调用方 (老 client / write-timeout 回调) 完全不变；新功能要靠 client 显式 传非空 block_hashes 才能生效。 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… forward to manager 为任务 82620492 在 MetaServiceImpl::FinishWriteCache 解析 client 上报的 block_hash 并透传到 cache_manager： - 从 request->locations() 收集每个 location 的 block_hash 组成 vector<int64_t>， 传给 CacheManager::FinishWriteCache 的新增参数。 - 协议约定 client 发 locations.size() == keys.size() (即 StartWriteCache 时所有 block_keys，无论 success/fail，按顺序对齐)；失败的 block 也带占位 CacheLocation 但 block_hash=0。 - 老 client 不传 locations 时 vector 为空，cache_manager 不会修改 CacheLocation 上已有的 hash (向后兼容)；长度不匹配的非法情况由 CacheManager 统一返回 EC_BADARGS。 - 本 commit 不加 service 层 UT：透传逻辑在 manager/test 的 TestBatchUpdateLocationStatusPersistsBlockHash 已覆盖；端到端覆盖留到 commit 8/10 的 client 单测。 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
为任务 82620492 加入 event 类型 ChecksumMismatchEvent，用于读端发现 checksum mismatch 时的审计 / 反向追溯。结构与 cache_reclaim_event.h / optimizer_event.h 一致，承载 instance_id、block_key、expected/actual hash、spec_name、storage_uri 等字段。 本 commit 仅引入类定义；事件发布通路 (client 在 LoadKvCaches 检测到 mismatch 时调用 event_manager_->Publish) 在 commit 8 client 实现时接入。 不在本 commit 同时加 DataStorageMetricsCollector 的 checksum_mismatch counter： (1) mismatch 信号源在 client 端，server 端没有专门的触发点； (2) client SDK 目前没有现成的 MetricsCollector 基础设施。 等 commit 8 把 client 端的事件 + log 落地后，再按需补充 metrics counter (可能放在 client 端的新 collector，或在 server 加专门的 mismatch 上报接口 触发现有 ServiceMetricsCollector)。 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… at Init
为任务 82620492 在 TransferClient SDK 层落地方案 A 的写 / 读校验通路 +
方案 B 的启动期拒绝防线：
- TransferClient::SaveKvCaches 加 default std::vector<int64_t> *
out_block_hashes = nullptr：非空时 SDK 算出与 block_buffers 1:1 对齐的
hash 列表写入。
- TransferClient::LoadKvCaches 加 default const std::vector<int64_t> *
expected_hashes = nullptr：非空时拉数据后算 actual 并按下标比对，
expected[i]==0 视为 sentinel 跳过 (兼容老数据 / 老 client)；任一
block 不匹配返回 ER_CHECKSUM_MISMATCH。
- 老 caller 完全不变，新 caller 显式传 vector 才触发。
- TransferClientImpl::Init 新增 ValidateStorageConfigsForIntegrity：
parse init_params.storage_configs 检查每个 spec 的 integrity 字段，
enable_inline_header=true -> ER_INLINE_HEADER_INVALID
inline_header_version != 0 -> ER_INLINE_HEADER_INVALID (孤儿 version)
任一 spec enable_meta_checksum=true -> 自动初始化 sdk_buffer_check_pool
非 CUDA/MUSA build 时校验路径整体退化为 no-op (warn 日志)。
- 现有 KVCM_SDK_CHECK 路径 (print-only) 保留作为 debug fallback，
与新通路共用同一个 pool，hash 只算一次。
- transfer_client_test 加 5 个 case：拒绝 inline_header、拒绝 orphan
version、接受 meta_checksum=true、expected 全 0 跳过、长度不匹配。
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…Write
为任务 82620492 把 SDK 算出的 block_hash 从 TransferClient 一路打到
FinishWriteCacheRequest 的 wire 上：
- ManagerClient::FinishWrite / MetaClient::FinishWrite 加 default
const std::vector<int64_t> & block_hashes = {}；
Save/LoadKvCaches 加 default 指针参数 expected/out_block_hashes，
与 TransferClient 接口对齐。
- ManagerClientImpl / MetaClientImpl 透传，最终 GrpcStub::FinishWriteCache
把 block_hashes[i] 填到 proto::meta::FinishWriteCacheRequest.locations[i]
.block_hash；长度与 locations 不一致时 warn + 跳过 hash 写入 (server 端
会再次校验长度)。
- Stub 基类 / GrpcStub / MockStub 同步加签名；MockStub 之前缺 override
会让 abstract class new() 编译失败。
- pybind11 binding 暂时用 lambda 截断到老 3 参数 (Python 端不暴露 hash)，
等 py_connector 接入 checksum 时再扩展 Python 签名。本期范围决策只动
KVCM 仓库，连同 Python binding 一起保持向后兼容。
- RTPLLMClient 这条 client 路径接口保持不变 (它内部调 manager_client
时走 default 参数，行为同老版本)。
老 caller 完全不变；新路径要靠新 caller 显式传非空 block_hashes 才能
触发，与之前 commit 5/6 (server 端透传) 形成闭环。
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…eservation) 为任务 82620492 加入设计文档 docs/design/data_integrity.md，描述: - 方案 A (meta 校验通路) 的完整流程图、API 变化、Validate 三层防线、 兼容性 sentinel (block_hash=0 sky 跳过)、容错与故障表现矩阵。 - 方案 B (inline header) 当前的接口预留状态、启动期拒绝防线、未来落地 时只需删两条拒绝分支不动 wire format 的兼容路径。 - 9 个 commit 的拆分映射 (commit 10 chaos test 显式跳过，理由见 follow-up)。 - 后续工作清单 (端到端 chaos test、py_connector 接入、metrics 接入、 方案 B 真实现、多副本一致性)。 - 关键文件索引。 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Codex flagged that RegistryManager::AddStorage and RecoverStorageUnsafe went straight to DataStorageManager::RegisterStorage without ever running StorageConfig::ValidateRequiredFields. Any invalid config — enable_inline_ header=true, orphan inline_header_version, or enable_meta_checksum with algo=CA_UNSPECIFIED — would land in the registry silently, get persisted, and reappear on every restart. Worker Init would then reject it, so the server said the config was OK but every client disagreed. Both entry points now call ValidateRequiredFields: * AddStorage returns EC_BADARGS and logs the invalid fields before touching the registry storage. * RecoverStorageUnsafe treats a validation failure the same as any other recovery failure: log, increment error_count, skip that entry (so one bad persisted config does not block recovery of the rest). Test: TestAddStorageRejectsInvalidIntegrity in registry_manager_local_ backend_test.cc covers the three invalid variants (inline_header=true, orphan version, enable_meta_checksum with CA_UNSPECIFIED) plus a default-integrity sanity case that must still succeed. External source: registry_manager_local_backend_test passes under ASAN debug. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Codex flagged that StorageConfig::FromRapidValue ignored the return value of integrity_.FromRapidValue. A malformed integrity block — e.g. "enable_meta_checksum": "true" as a string rather than a bool — silently reset integrity to defaults (everything disabled) and the parent StorageConfig::FromRapidValue still returned true. The operator got an unenforced config where checksum-verification was actually off. Fix: propagate the nested parser's return value. If the "integrity" object exists and any field has the wrong type, the whole StorageConfig JSON parse fails and the caller (RegistryManager, TransferClient Init, admin service converters) will refuse the config instead of running with a partial view. Test: TestStorageConfigRejectsMalformedIntegrity feeds a string-typed "enable_meta_checksum" and asserts FromJsonString returns false. External source: StorageConfigTest passes under ASAN debug. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Sync the design doc with the second review-feedback round: - Fast path description: aggregate now multiplies each block by (2*i+1)*kIndexSalt before XORing so block swaps and same-delta paired mutations are both caught in the fast path. strict_mode is retained purely as a diagnostic knob for on-call triage. - Validate section renamed 三 → 四 层防线; explicitly lists the new RegistryManager Add / Recover validation and the StorageConfig malformed-integrity JSON rejection. - Fault matrix picks up six additional rows covering block swap / same-delta mutation, RegistryManager rejection of bad persisted configs, StorageConfig JSON type rejection, SaveKvCaches IOV overflow, out_checksums cleared on Put failure, and merged CacheLocation checksum round-trip. - Commit roster extended from 13 to 20; entries 14-20 cover the two rounds of review-feedback commits and this docs update. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
5bd7e8f to
e4645fe
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4645fe6e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (request->locations_size() > 0) { | ||
| checksums.reserve(request->locations_size()); | ||
| for (const auto &loc : request->locations()) { | ||
| checksums.push_back(loc.checksum()); |
There was a problem hiding this comment.
When a client sends the pre-existing locations field but does not support checksums, every proto location has the default checksum=0, yet this branch still builds a non-empty checksums vector. CacheManager::FinishWriteCache then treats that as an explicit checksum report and rejects it unless its length equals the full StartWrite key count, so legacy C++ callers that populated FinishWrite's existing Locations argument with only written/successful locations can now fail partial writes with EC_BADARGS. Only populate this vector when checksum data was actually supplied, or preserve the old behavior when the request only contains legacy locations.
Useful? React with 👍 / 👎.
| return std::all_of(block_buffer.iovs.begin(), block_buffer.iovs.end(), [](const Iov &iov) { | ||
| return !iov.ignore && iov.base != nullptr && iov.size > 0; |
There was a problem hiding this comment.
When enable_meta_checksum is used in a CUDA/MUSA build with CPU-backed Iovs (which the local, HF3FS, and Mooncake SDK paths support), this predicate still marks the block hashable because it only checks ignore/base/size. The checksum path then passes those raw host pointers into SdkBufferCheckUtil, whose device kernel dereferences iov.base as GPU memory, so otherwise valid CPU-buffer reads or writes can fail or crash as soon as checksum verification is enabled; either require GPU/MUSA IOVs here or add a CPU checksum path/skip.
Useful? React with 👍 / 👎.
| // Deliberately DO NOT assign to *out_checksums here — see below. A prior | ||
| // version filled the vector before Put, which meant a failed Put returned | ||
| // checksums for data that never landed on disk; a caller then persisting | ||
| // those via FinishWrite would associate a checksum with a nonexistent block. |
There was a problem hiding this comment.
When the checksum pool is enabled and hashing succeeds but sdk_wrapper_->Put() later fails (for example an invalid URI or backend write error), this deferred assignment leaves any existing contents of *out_checksums untouched. A caller that reuses the vector can therefore carry stale checksums from a previous successful save into FinishWrite despite this save failing; clear out_checksums before the precompute or on every Put failure path.
Useful? React with 👍 / 👎.
| if (!any_compared || expected_xor == actual_xor) { | ||
| return result; // all match (or nothing to check) |
There was a problem hiding this comment.
Fresh evidence after the multiplier change: because every per-index multiplier is odd, flipping bit 0x8000000000000000 in an even number of actual checksums preserves the aggregate ((x ^ high_bit) * odd == (x * odd) ^ high_bit, and the XOR cancels), so this fast-path return can still report success without entering the per-block fallback even though multiple buffers are corrupt. Compare per block before accepting, or use an aggregate that cannot deterministically cancel this high-bit delta.
Useful? React with 👍 / 👎.
| KVCM_LOG_ERROR("block [%zu] has invalid iov_num [%zu] for checksum hash", i, iov_num); | ||
| return false; | ||
| } | ||
| if (!chunk.empty() && (iov_num != chunk_iov_num || chunk_total_iovs + iov_num > max_check_iov_num)) { |
There was a problem hiding this comment.
When checksum-enabled reads or writes contain blocks with the same IOV count but different IOV sizes, this chunking sends them together to SdkBufferCheckUtil::GetBlocksHash. That helper derives cal_byte_size from the first IOV only and the GPU kernel applies it to every IOV, so a later smaller IOV in the chunk can be read past its buffer (or make hashing fail) even though the SDKs otherwise support variable-size IOVs; split by compatible sizes or validate all IOVs against the chosen checksum window before hashing.
Useful? React with 👍 / 👎.
8c275ee to
bc071b7
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc071b781c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| reserved 1; // historical cluster_name, removed from the API but tag must not be reused | ||
| reserved "cluster_name"; | ||
| int64 heartbeat_timeout_ms = 2; | ||
| int64 cleanup_grace_ms = 3; | ||
| int64 liveness_check_interval_ms = 4; |
There was a problem hiding this comment.
This changes existing protobuf wire numbers: in the parent commit heartbeat_timeout_ms, cleanup_grace_ms, and liveness_check_interval_ms used tags 1/2/3, but this reserves tag 1 and moves the fields to 2/3/4. During rolling upgrades or with any existing admin client/server, Vineyard storage configs are silently misdecoded (for example, an old client's heartbeat timeout on tag 1 is dropped, while a new tag-2 heartbeat is read as cleanup grace by old clients), so liveness timing can be reset or swapped. Keep the existing field numbers and reserve only truly unused/removed tags.
Useful? React with 👍 / 👎.
| if (out_checksums != nullptr && block_checksums_computed) { | ||
| *out_checksums = std::move(block_checksums); |
There was a problem hiding this comment.
Because *out_checksums is only assigned after a successful Put, a caller that reuses a non-empty vector can still see stale checksums when hashing succeeds but sdk_wrapper_->Put fails above (bad URI, timeout, backend error). In checksum-enabled CUDA/MUSA builds this lets the caller accidentally forward old checksums to FinishWrite for a save that did not commit, which is the case this path is trying to prevent; clear out_checksums before every Put-error return or at function entry.
Useful? React with 👍 / 👎.
| return std::all_of(block_buffer.iovs.begin(), block_buffer.iovs.end(), [](const Iov &iov) { | ||
| return !iov.ignore && iov.base != nullptr && iov.size > 0; | ||
| }); |
There was a problem hiding this comment.
When enable_meta_checksum is used in a CUDA/MUSA build with CPU BlockBuffer iovs (the public API supports MemoryType::CPU, and local-file/NFS tests use CPU buffers), this predicate marks those iovs hashable and sends their host pointers to SdkBufferCheckUtil, whose GPU kernel dereferences iov.base as device-accessible memory. Pageable host pointers can produce illegal-address failures instead of a clean skip/reject; require iov.type == MemoryType::GPU here or add a CPU checksum path.
Useful? React with 👍 / 👎.
| if (has_checksums && checksums.size() != location_info.keys.size()) { | ||
| RETURN_IF_EC_NOT_OK_WITH_LOG(WARN, | ||
| EC_BADARGS, | ||
| "checksums size (%zu) does not match keys size (%zu)", | ||
| checksums.size(), |
There was a problem hiding this comment.
When a new/HTTP caller sends a non-empty checksum list whose length differs from the write session's keys, this branch runs after GetAndDelete has already removed the write session, then returns before either marking successes or submitting failed_del_request. The client cannot retry FinishWriteCache, and any allocated CLS_NEW locations/storage for that session are left unreclaimed until some unrelated cleanup path finds them; reject without deleting the session or explicitly reclaim all session locations on this error path.
Useful? React with 👍 / 👎.
77b7710 to
0075d12
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a32e8bc98
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // the server commits the write with checksum=0 while the caller believes the | ||
| // checksum was reported and an undetectable read corruption window opens. | ||
| if (!checksums.empty()) { | ||
| if (static_cast<int>(checksums.size()) != proto_locations->size()) { |
There was a problem hiding this comment.
After the final change this now returns ER_INVALID_PARAMS whenever a caller reports the full checksum vector but still passes the legacy locations argument as empty or only the successful locations; those call patterns exist because the server-side write session already owns the allocated location IDs and FinishWriteOptions documents checksums as parallel to the StartWrite batch, not to this legacy argument. In that scenario SaveKvCaches can succeed and produce checksums, but the write cannot be committed with integrity unless the caller fabricates placeholder CacheLocations, so the stub should synthesize full-length proto slots or avoid validating against proto_locations->size().
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99e4a79b40
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| static LoadKvCachesOptions VerifyWith(const std::vector<int64_t> &checksums) { | ||
| LoadKvCachesOptions options; | ||
| options.expected_checksums = &checksums; | ||
| return options; |
There was a problem hiding this comment.
When callers build options from a temporary or returned vector, for example LoadKvCachesOptions::VerifyWith(GetChecksums()), the const std::vector<int64_t>& parameter binds to that temporary but the option stores its address; after the factory returns, TransferClientImpl::LoadKvCaches dereferences a dangling pointer. Make these factories require an lvalue reference or have the option own/copy the vector; FinishWriteOptions::WithChecksums has the same lifetime issue.
Useful? React with 👍 / 👎.
| auto iov_shape = GetIovSizeShape(block_buffers[i]); | ||
| if (!chunk.empty() && (iov_shape != chunk_iov_shape || chunk_total_iovs + iov_num > max_check_iov_num)) { |
There was a problem hiding this comment.
Fresh evidence after shape grouping: a single block whose shape is [large, small] still enters one GetBlocksHash call because this only compares whole-block shapes, but SdkBufferCheckUtil::GetIovsCrc derives cal_byte_size from the first IOV and applies it to every IOV. With a later IOV smaller than that window, such as a tiny latent slice or a larger KVCM_CHECK_IOV_BYTE_SIZE, checksum-enabled Save/Load can read past that buffer or fail even though the SDK transfer itself supports the IOVs; validate every IOV against the chosen window or split/hash them safely.
Useful? React with 👍 / 👎.
| // Round-trip the checksum so readers can verify with the value persisted at write | ||
| // time. 0 means "no checksum" and is the legacy default; new readers skip the | ||
| // sentinel. | ||
| proto_cache_location->set_checksum(cache_location_info.checksum()); |
There was a problem hiding this comment.
When GetCacheLocation is called with location_spec_names, CacheManager::FilterLocationSpecByName has already removed the unrequested specs before this serializer runs, but this still returns the full-block checksum. A checksum-enabled caller that collects that value and reads only the returned specs will hash a partial buffer against the full-block checksum and get ER_CHECKSUM_MISMATCH for otherwise valid partial/spec-filtered reads; emit the zero sentinel or a matching partial checksum when specs have been filtered.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38cbd2529a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| int64 heartbeat_timeout_ms = 2; | ||
| int64 cleanup_grace_ms = 3; | ||
| int64 liveness_check_interval_ms = 4; |
There was a problem hiding this comment.
This renumbers existing admin API protobuf fields: in the parent schema heartbeat_timeout_ms, cleanup_grace_ms, and liveness_check_interval_ms were tags 1/2/3, but they are now 2/3/4. During a rolling upgrade or with any client generated from the previous proto, a Vineyard AddStorage/UpdateStorage request will decode as heartbeat=old cleanup, cleanup=old liveness, and liveness=default, silently corrupting the liveness configuration. Please keep the existing tags and only reserve truly unused/removed tag numbers.
Useful? React with 👍 / 👎.
| // Same block replicated across specs shares one checksum by design (see | ||
| // data_integrity design doc); carry the winner's so the read path can verify. | ||
| result->set_checksum(winner->checksum()); |
There was a problem hiding this comment.
When a block has complementary locations from location_spec_group_names (the write path explicitly allows KV-only/Mamba-only locations to coexist), this merge builds a location from multiple stored entries but returns only the winner's checksum. A reader that requests checksums for the merged result can then verify data read from the non-winner specs against the wrong expected value, producing false ER_CHECKSUM_MISMATCH or leaving part of the merged data unchecked; suppress the checksum for merged heterogeneous locations or carry checksums at the same granularity as the data being returned.
Useful? React with 👍 / 👎.
| std::string algo_str; | ||
| KVCM_JSON_GET_DEFAULT_MACRO(rapid_value, "algo", algo_str, std::string("crc32_xor_int64")); | ||
| algo_ = ToChecksumAlgo(algo_str); |
There was a problem hiding this comment.
For JSON storage configs, integrity: {"enable_meta_checksum": true} is accepted because a missing algo is filled in as crc32_xor_int64 before ValidateRequiredFields() runs. The admin-proto path leaves an omitted enum as CA_UNSPECIFIED and rejects it, and the validation contract says checksum-enabled configs must set the algorithm explicitly; this inconsistency lets a typo/omission silently enable CRC instead of failing configuration validation.
Useful? React with 👍 / 👎.
| // Round-trip the checksum so readers can verify with the value persisted at write | ||
| // time. 0 means "no checksum" and is the legacy default; new readers skip the | ||
| // sentinel. | ||
| proto_cache_location->set_checksum(cache_location_info.checksum()); |
There was a problem hiding this comment.
When location_spec_names is non-empty, CacheManager::FilterLocationSpecByName has already removed part of the block's location_specs, but this still serializes the original full-block checksum. Clients using MatchLocationOptions::WithChecksums for that subset will pass a full-block expected checksum to LoadKvCaches, which hashes only the returned subset unless the caller has separate ignored IOVs, causing false checksum mismatches; clear the checksum or expose checksums at the same granularity as the returned specs.
Useful? React with 👍 / 👎.
| static FinishWriteOptions WithChecksums(const std::vector<int64_t> &checksums) { | ||
| FinishWriteOptions options; | ||
| options.checksums = &checksums; |
There was a problem hiding this comment.
These helpers accept a const std::vector<int64_t>& but store its address in the options object. Calls that build options inline from a temporary, such as LoadKvCachesOptions::VerifyWith({1, 2}) or a temporary SaveKvCaches(...).second.checksums, leave a dangling pointer before LoadKvCaches/FinishWrite reads it, which can send garbage checksums or report random mismatches; make the options own the vector or reject rvalue inputs.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 23d329253a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| int64 heartbeat_timeout_ms = 2; | ||
| int64 cleanup_grace_ms = 3; | ||
| int64 liveness_check_interval_ms = 4; |
There was a problem hiding this comment.
When admin clients and servers are upgraded independently, shifting VineyardStorageSpec from tags 1/2/3 to 2/3/4 changes the protobuf wire contract: an older client still sends heartbeat_timeout_ms on tag 1, which the new server now discards, while tags 2 and 3 are decoded as the wrong fields. This can register Vineyard storage with default or swapped timeout settings; keep the existing field numbers and only add/reserve truly removed fields with unused tags.
Useful? React with 👍 / 👎.
| return false; | ||
| } | ||
| return std::all_of(block_buffer.iovs.begin(), block_buffer.iovs.end(), [](const Iov &iov) { | ||
| return !iov.ignore && iov.base != nullptr && iov.size > 0; |
There was a problem hiding this comment.
This helper treats any non-null IOV as hashable, but the checksum path below uses SdkBufferCheckUtil, whose CUDA/MUSA kernel dereferences iov.base as device memory. In CUDA/MUSA builds, requesting checksums for CPU-backed buffers (for example local-file or HF3FS CPU IOVs) passes this check and can hit an illegal device address instead of returning a clean error, so either require MemoryType::GPU here or route CPU IOVs through a CPU hash path.
Useful? React with 👍 / 👎.
| // An empty vector means the client did not report any; existing CacheLocation | ||
| // checksums are then preserved. Any length mismatch is treated as a client bug. | ||
| const bool has_checksums = !checksums.empty(); | ||
| if (has_checksums && checksums.size() != location_info.keys.size()) { |
There was a problem hiding this comment.
When StartWriteCache filters an existing prefix or already-cached blocks, the write-session state contains only new_keys, but the new API/design says FinishWriteOptions::checksums is the full StartWrite batch with zeroes for skipped positions. This length check compares against the filtered location_info.keys, so a checksum-enabled caller following that contract gets EC_BADARGS whenever some blocks were skipped and no checksum is persisted for the blocks that were actually written.
Useful? React with 👍 / 👎.
| result->set_location_specs(std::move(specs)); | ||
| // Same block replicated across specs shares one checksum by design (see | ||
| // data_integrity design doc); carry the winner's so the read path can verify. | ||
| result->set_checksum(winner->checksum()); |
There was a problem hiding this comment.
During rolling upgrades or when a new replica is added by a legacy/no-checksum writer, replicas for the same block can be mixed with checksum == 0 and a real checksum. Since the selection policy is weighted rather than checksum-aware, if the zero-checksum replica wins here the merged location returns 0 and the read path skips verification even though another merged replica had a usable checksum; prefer the nonzero checksum (or reject inconsistent nonzero values) when building the merged view.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 09c13301cc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| repeated CacheLocation locations = 5; | ||
| // Full-batch checksum vector, parallel to the keys captured by StartWriteCache. | ||
| // Empty = not reported; 0 per entry = no checksum for that block. | ||
| repeated int64 checksums = 5; |
There was a problem hiding this comment.
The previous schema used tag 5 for repeated CacheLocation locations, and pre-upgrade clients still populate that field on FinishWriteCache. Because proto3 repeated int64 is packed by default, a new server will parse those old length-delimited CacheLocation bytes as checksums instead of ignoring them; MetaServiceImpl then forwards a non-empty vector that either fails the length check or stores bogus checksums if it happens to align. This breaks rolling upgrades/old SDK compatibility despite the documented “old client” path; add checksums on a fresh field number and reserve the old locations tag instead.
Useful? React with 👍 / 👎.
| int64 heartbeat_timeout_ms = 2; | ||
| int64 cleanup_grace_ms = 3; | ||
| int64 liveness_check_interval_ms = 4; |
There was a problem hiding this comment.
For admin clients generated from the previous proto, heartbeat_timeout_ms, cleanup_grace_ms, and liveness_check_interval_ms are sent as fields 1, 2, and 3. After this renumbering, a mixed-version AddStorage/UpdateStorage request drops the old heartbeat value and shifts cleanup into heartbeat and liveness into cleanup when manager_message_proto_util.cc reads the new fields, so Vineyard liveness/cleanup timing can be misconfigured during upgrades. Keep the existing field numbers and only add new fields at unused tags.
Useful? React with 👍 / 👎.
2 participants
Summary
Introduce end-to-end data integrity checks on the KVCM read/write path.
The change ships Scheme A (block hash persisted in meta and verified on
read) and reserves the wire format for Scheme B (inline header on
stored blocks) so a follow-up implementation can land without further
protocol churn.
Internal tracking: Aone task
82620492.Motivation
The client SDK already carries
SdkBufferCheckUtil::GetBlocksHash(GPU-accelerated CRC32 →
int64block hash), but it is currently gated bythe
KVCM_SDK_CHECKenvironment variable and only prints the hash to alog file. Detecting a real read corruption requires a human grepping
both ends — there is no production-grade signal that a block was read back
incorrectly (data rot, mis-routed location, sharded buffer reuse, etc.).
Design at a glance
Full design with API tables, validation invariants, and failure matrix
lives in
docs/design/data_integrity.md.What's in this PR
Public API changes (all additive, fully backwards-compatible)
TransferClientSaveKvCachesstd::vector<int64_t>* out_block_hashesnullptrTransferClientLoadKvCachesconst std::vector<int64_t>* expected_hashesnullptrManagerClient/MetaClientFinishWriteconst std::vector<int64_t>& block_hashes{}Stub/GrpcStubFinishWriteCache{}CacheManagerFinishWriteCache{}MetaSearcher::LocationUpdateTaskblock_hashfield0Every old call site continues to compile and runs without behavioural
change. New clients opt in by passing a non-empty vector / non-null pointer.
Wire format
DataIntegrityConfigmeta_service.proto,admin_service.proto,kv_meta_service.protoChecksumAlgoenumCA_CRC32_XOR_INT64, mapping 1:1 to the existing SDK implementationStorageConfig.integrity11, optionalCacheLocation.block_hash(int64)meta_service.proto,admin_service.proto4;0is the «not set» sentinel(EC/ER/proto)_CHECKSUM_MISMATCH/_INLINE_HEADER_INVALIDToPbError+ToClientErrorFinishWriteCacheRequest.locationswas already a reserved slot in theproto; this PR reuses it to carry the per-block hash from client to
server, so no new RPC is introduced.
Compatibility sentinel
block_hash == 0is a deliberate sentinel meaning «no hash for thisblock», honoured at three boundaries:
LocationUpdateTask.block_hash = 0→BatchUpdateLocationStatusdoesnot overwrite the existing hash on
CacheLocation.CacheLocation.block_hash = 0→ legacy data / legacy client; readerskips verification.
FinishWriteCacheRequest.locations[i].block_hash = 0→ server stores0, same semantics.False negative rate (a genuine hash happening to be
0) is on the orderof
2^-64after the XOR aggregation — accepted as a tradeoff to avoidintroducing 3-state semantics on a primitive type.
Scheme B reservation
enable_inline_header=trueandinline_header_version != 0(without theflag) are explicitly rejected at two layers:
StorageConfig::ValidateRequiredFields— rejects at server startup.TransferClientImpl::Init— rejects on client init withER_INLINE_HEADER_INVALID.This prevents the «proto flag is on but nothing happens» silent failure
mode. When Scheme B is implemented in a follow-up, only the two rejection
branches need to come out — no proto or API surface changes.
Validation invariants (
DataIntegrityConfig::ValidateRequiredFields)enable_inline_header=true→ rejected (not implemented in this release).inline_header_version != 0withoutenable_inline_header=true→ rejected (orphan version).enable_meta_checksum=truewithalgo == CA_UNSPECIFIED→ rejected (algorithm must be explicit).Commits
Eleven commits, ordered so each one is independently reviewable and
exercises its package's UT in isolation:
[protocol] add DataIntegrityConfig and CacheLocation.block_hash for read/write integrity check[common] add CHECKSUM_MISMATCH / INLINE_HEADER_INVALID error codes across layers[data_storage] persist DataIntegrityConfig in StorageConfig and reject inline_header[meta] carry block_hash on CacheLocation with backward-compatible JSON[manager] plumb block_hash through MetaSearcher and CacheManager::FinishWriteCache[service] parse block_hash from FinishWriteCacheRequest.locations and forward to manager[event] add ChecksumMismatchEvent class for data integrity reporting[client] expose block_hash on TransferClient and reject inline_header at Init[client] plumb block_hash through ManagerClient and MetaClient FinishWrite[docs] add design doc for data integrity check (scheme A + scheme B reservation)[manager] update storage JSON literal assertions for new integrity fieldTest plan
ASAN debug runs (
bazelisk test --config=debug --config=asan --test_env ASAN_OPTIONS=detect_odr_violation=0 ...) on both source modes:17 newly added cases pass. Highlights:
data_storage/test:StorageConfigTest— 5 new cases covering defaults,JSON round-trip, inline-header rejection, orphan-version rejection,
legacy backward compatibility, validation propagation.
meta/test:cache_location_test— 4 new cases covering default value,JSON round-trip, legacy JSON missing
block_hashdefaults to0,negative hash preserved.
manager/test:MetaSearcherTest— 2 new cases covering hashpersistence via
BatchUpdateLocationStatusand zero-sentinelpreserves the existing value.
client/test:TransferClientTest— 5 new cases coveringInit-time rejection ofenable_inline_header, rejection of orphaninline_header_version, acceptance ofenable_meta_checksum=true,sentinel-all-zero short-circuits, length mismatch returns
ER_CHECKSUM_MISMATCH.data_storage/test,meta/test,manager/test,service/test,client/test, andclient/src/internal/stub/test: 37/37 pass.backend → read expects
ER_CHECKSUM_MISMATCH) — not in this PR; needs aCUDA/MUSA runtime with a real device. Tracked in the follow-up list.
py_connectorintegration — not in this PR by scope decision;Python bindings are intentionally truncated to the legacy 3-argument
signature via a
lambdawrapper so vLLM / SGLang / TRT-LLM connectorskeep building unchanged.
Performance impact
enable_meta_checksum=true, one GPU hash computeper
SaveKvCachesbatch (reuses the existing pool and warm-up). Whendisabled, zero overhead — the new vector arguments are
nullptrandthe branch is short-circuited.
LoadKvCachesbatch whenverification is enabled; otherwise no-op.
block_hashadds 8 bytes perCacheLocationinthe meta indexer (Redis / Local). No new RPC, no extra round-trips.
FinishWriteCacheRequest.locations[i]already serialized;the new
block_hashadds 8 bytes per entry when set.Backwards compatibility matrix
block_hash = 0, no change in semanticsblock_hashrides on the reservedlocationsfield; old server silently ignoresblock_hash = 0in meta)integrityfield in JSONenable_inline_header = trueanywhereFollow-ups (tracked in design doc)
integration_test/client_test/.py_connectoradoption (Python binding extension + vLLM / SGLang / TRT-LLM hash plumbing).checksum_mismatch_counter; the signallives in the client and we don't yet have a client-side
MetricsCollector. Options: add a client-side collector, or introducea dedicated
RemoveCacheByMismatchRPC so the existingServiceMetricsCollectorcan record on the server.sdk_wrapperbuffer-layout rework,capacity accounting, lazy migration.
that the client computes one hash per block shared across all replicas;
needs to be exercised in chaos.