[Bugfix][Store] Reject invalid metadata client IDs#2990
Conversation
Reject malformed client_id values while deserializing object metadata so corrupted snapshot entries are not restored with an invalid UUID. Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Code Review
This pull request adds validation for the client ID UUID during metadata deserialization in MasterService, returning a serialization error if the conversion fails. It also introduces a unit test to verify this behavior and refactors the test utilities to support payloads with invalid client ID strings. There are no review comments, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Apply the format expected by the repository clang-format check. Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
Use clang-format 20.1.8 output for the invalid metadata client ID regression test. Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
Pack the invalid client_id regression test payload with the numeric shard key shape expected by MetadataSerializer::Deserialize(). Co-authored-by: chatgpt-codex-connector[bot] <199175422+chatgpt-codex-connector[bot]@users.noreply.github.com>
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Description
Reject malformed
client_idvalues whenMasterService::MetadataSerializerdeserializes object metadata.What Problem This Solves
DeserializeMetadata()read the serializedclient_idstring and calledStringToUuid(client_id_str, client_id), but ignored the boolean result. If snapshot metadata contained a malformed client ID such asnot-a-uuid, deserialization continued and could constructObjectMetadatawith an invalid UUID value instead of treating that entry as corrupt metadata.A nearby sibling path already handles this strictly: the catalog-backed snapshot provider checks
StringToUuid()and returnsDESERIALIZE_FAILfor invalid client UUID strings. Replica msgpack deserialization also rejects invalid client IDs. This PR brings the master metadata deserializer in line with those paths.Changes
StringToUuid()inDeserializeMetadata().DESERIALIZE_FAILwith a targeted error message when the serialized metadata client ID is malformed.client_id = not-a-uuidand verifies the object key is not restored into metadata.The existing shard-level behavior is intentionally preserved:
DeserializeShard()logs a per-object metadata deserialization failure and continues with the remaining entries.Evidence
Before this change, the invalid client ID parse result was ignored:
The new test exercises the restore deserialization entry point with a malformed client ID payload:
and asserts that the bad entry is skipped instead of being restored:
EXPECT_FALSE(KeyExistsInMetadata(service_.get(), key));Possible call chain / impact
This affects the master metadata restore/deserialization path for malformed persisted metadata. It does not change serialization output, valid UUID handling, replica parsing, or the shard-level policy that skips individual corrupt object entries.
Module
mooncake-transfer-engine)mooncake-store)mooncake-ep)mooncake-pg)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-common)mooncake-rl)Type of Change
How Has This Been Tested?
Test commands:
git diff --check cmake -S . -B build-store-test -DBUILD_UNIT_TESTS=ON -DWITH_STORE=ON -DWITH_TE=ON -DWITH_STORE_RUST=OFF -DWITH_EP=OFF -DUSE_CUDA=OFF -DUSE_ETCD=OFF -DSTORE_USE_ETCD=OFF -DSTORE_USE_REDIS=OFF -DCMAKE_BUILD_TYPE=RelWithDebInfoTest results:
git diff --checkpassed. The focused CMake configure was attempted in local WSL, but this machine is missing theyaml-cppdevelopment package, so the local build/test target could not be configured here.Checklist
./scripts/code_format.shpre-commit run --all-filesand all hooks pass./scripts/code_format.sh/ pre-commit were not run locally because the Windows checkout exposes the shell script with CRLF line endings in WSL andpre-commitis not installed in this environment.AI Assistance Disclosure
Used ChatGPT Codex to inspect the deserialization path, implement the small guard and regression test, and draft this PR description. The submitter is responsible for reviewing and defending the change.