Conversation
… payload layout in GetReplica
he-yufeng
left a comment
There was a problem hiding this comment.
Read it line by line and ran it in the dev container. This is right and complete.
The validator closes all three failure modes from the issue: exact per-entry equality against the stored SizeList rejects both over- and under-sized buffers before any registration or transfer, the MaxShardSize == 0 arm removes the divide-by-zero, and the shard-count identity catches the nastier case where stored SizeList and stored Shards disagree with each other (the truncatedShards case, which size equality alone would wave through). Placement is correct in both spots: GetReplica right after the first metadata.Get, and updatePayloadMetadata revalidates on the re-fetched payload after a conflict, closing the metadata-changed-mid-transfer window.
The tests assert the fail-closed ordering properly: no memory registration and no metadata update happen when the check fires, which is the part that keeps a rejected call from advertising a partial replica.
Verified locally (dev container, Go 1.25.7, freshly built transfer_engine/base/mooncake_common): TestGetReplicaRejectsCallerLayoutMismatch all six subcases pass on this head, as does the pre-existing layout-change suite; the new test fails on the parent commit without the fix (panic, as the issue predicts). CI is green on the PR.
One non-blocking note for a follow-up, not this PR: the shipped example still derives sizeList from a hardcoded file_size_mb rather than the layout from List(), so it exercises none of this. Not your problem to fix here, just worth recording.
Good to land from my reading.
Description
GetReplicaonly checked thataddrListandsizeListhad the same non-zero length. It never comparedsizeListwith the layout stored in etcd for the payload, butdoGetReplicaandupdatePayloadMetadatawalk the caller's sizes inMaxShardSizesteps and indexpayload.Shardsby position, transferring the stored shard length into each address.With a mismatched
sizeListthis meant:payload.Shards[taskID]runs past the slice and the process panics;GetReplicareturnsnil, and the node advertises itself as a replica source for a partial copy.This PR adds
validateReplicaLayout, which returnsErrInvalidArgumentunless the caller'ssizeListmatches the storedSizeListentry for entry,MaxShardSizeis non-zero, and the implied shard count equalslen(payload.Shards).GetReplicacalls it right after the first metadata read, before any memory is registered.updatePayloadMetadatacalls it after re-reading the payload on a CAS miss, since that path walks the shard list the same way.Fixes #4303
Module
mooncake-transfer-engine)mooncake-store)mooncake-conductor)mooncake-reshard)mooncake-ep)mooncake-pg)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-common)mooncake-rl)Type of Change
How Has This Been Tested?
Added
TestGetReplicaRejectsCallerLayoutMismatchinmetadata_recheck_test.go, using the existingscriptedMetadataandsuccessfulTransferEnginefakes. It covers six cases: larger than stored, smaller than stored, same total split differently, an extra buffer, a shard list shorter than the layout, and a zeroMaxShardSize. Each case assertsErrInvalidArgument, that no memory was registered, and that no metadata update was issued.Against the unpatched
core.gothe new test fails with the crash this PR fixes:With the fix, the whole package passes (10 tests).
gofmt -lreports nothing.go vetreports three pre-existingpossible misuse of unsafe.Pointerwarnings intransfer_engine.go, unrelated to this change.Note: the package tests need the transfer engine C library to link; they were run against a link-only stub of the C API since the tests only use Go fakes. CI does not currently run the Go tests.
Test commands:
Test results:
Checklist
./scripts/code_format.shAI Assistance Disclosure
Claude Code was used to fix the regression test.