Repository navigation
Conversation
|
Merged current main (6e847fb) so this picks up the two fixes the red legs were waiting on: #4513 restored the build broken by the Environ API removal, and #4440 pins the wheel-test torch to a shipped PG build. Rebuilt in the local docker toolchain and reran the full master_service_test, 68/68 green. The earlier failures were confined to those two repo-wide gaps, none of this branch. |
|
The red CI on the merge head was a real regression from this PR, not a main failure. Two loose ends of the deferred sweep, both fixed in b7faa93:
Local verification (Release build): |
|
The red wheel job is not from this diff. The same multi-rank TCP failure signature (ClientSession failing to read the WRITE ack frame with ECONNRESET, then mooncake_worker_thread rank DATA transfers to peer 3 erroring out) shows up in main's own CI: https://github.com/kvcache-ai/Mooncake/actions/runs/37930984424. This PR only touches the stale-handle sweep ordering in the store, nothing on the TCP session path. Rerunning the failed jobs; if the worker-thread test keeps flaking on main I will open an issue to track it separately. |
UnmountSegment runs ClearInvalidHandles inline, and the periodic sweep does the same from the task thread. Both erased a record as soon as every replica went stale, even when ExistKey had just granted a read lease on it, so a lookup -> load pair raced into OBJECT_NOT_FOUND on a key just reported as present, and a remount could no longer revive the bytes (kvcache-ai#4508). When a sweep round would finish a record (every replica stale or the metadata already invalid) and its read lease has not expired, leave the record in place; the next sweep after the lease lapses takes both. Partial cleanups that keep the record alive are unaffected, and the HA oplog REMOVE is skipped at the same decision point. Test: MasterServiceTest.SweepDefersToActiveReadLease reproduces the race (red without the gate, green with it), and UnmountSegmentKeepsSynchronousCleanupInHaMode now asserts the ExistKey lease pins the record through the unmount-triggered sweep. Full master_service_test: 68/68.
…s loadable CI on the merge head caught two things the deferred sweep dragged in. ClearStaleHandles' lease gate kept the record but left a promotion task anchored to it alive, so a dead holder kept the in-flight cap, the source refcnt and the reserved target replica until the lease lapsed. The delayed branch now cancels the key's promotion task on the spot. A retained record can also hold memory replicas whose allocator is already gone, and Serializer<AllocatedBuffer> needs the live allocator for the segment lookup, so snapshot persist failed the whole shard. Persist now leaves those replicas out but still writes the record itself, even when none remain: it restores invisible to reads (IsValid() is false) and the sweep reaps it once the persisted lease lapses, the same lifecycle as if no restart had happened, and the snapshot bytes round-trip identically. Local run: master_service_test 68/68, promotion_on_hit_test green, and master_service_test_for_snapshot 69/69, including the two cases CI lost (ClientExpiryClearsPromotionTask, CleanupStaleHandlesTest). Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>
b7faa93 to
4ef4869
Compare
|
Rebased onto current main and resolved the ObjectRecord restructure from #4508's follow-ups: the lease gate and the snapshot-serializable replica collection now read through |
…move kvcache-ai#4562 moved the in-flight promotion task from TenantState onto the record's route::ObjectState and kvcache-ai#4563 put tenant_id/user_key on the metadata, so the lease-gate cancel path no longer compiled against main. Follow the new layout: read the task off record.state, take source/tenant fields from the metadata, and reuse ErasePromotionTaskIfPresent for the quota refund and counters.
kvcache-ai#4588 renamed the cache tier member of CacheHitResult and RankCacheHitResult from npu to device but did not touch conductor_py.cpp, so the _conductor module stopped compiling and every CI run on main now fails at mooncake-integration/conductor/conductor_py.cpp:25. Point the two dict exports at the renamed member. Verified with a syntax-only compile of the translation unit: the pre-fix source fails with the same two "no member named 'npu'" errors seen in CI, the fixed source is clean. (cherry picked from commit 1dc4ca5)
|
Rebased onto current main. The red wheel builds were a semantic collision, not a flake: #4562 moved the in-flight promotion task from The tip commit is a cherry-pick of the |
Description
Fixes #4508.
The stale-handle sweep (
ClearInvalidHandles->ClearStaleHandles) erases object metadata without consulting the read lease. It is the one removal path that does not:Remove, both eviction passes, and quota reclaim all checkIsLeaseExpiredfirst.ExistKeygrants a lease on a hit but takes no refcnt, so a lookup -> load reader races the sweep:ExistKeyreports present, the sweep erases the record, and every laterGetReplicaListreturnsOBJECT_NOT_FOUND(-704) on a key that was a hit milliseconds earlier. Worse, the erase is permanent: when the client remounts andreplace_memory_bufferrebinds fresh buffers, the metadata that anchored the object is already gone, so the bytes on the remounted segment are orphaned. #4508 has this accumulating in production across three vLLM-Ascend clusters.With this change, when a sweep round would end the record (every replica stale, or the metadata already invalid) and the read lease is still live, the sweep skips the key and retries on a later pass after the lease expires. Two properties follow:
replace_memory_bufferrevalidates the same metadata and the object turns readable once the new buffers land, instead of being lost at the first sweep.GetReplicaListstill reportsOBJECT_NOT_FOUNDwhile handles are invalid (IsValid()gatesExists()), which is the truth at that moment. The fix turns a permanent loss into a transient failure bounded by the lease TTL, matching what every other removal path already guarantees.Partial cleanups are unchanged: when at least one valid replica survives, stale replicas are still dropped in the same pass. The gate sits at the erase decision under the shard write lock, the same point where
Removeand the eviction paths consult the lease, and it covers the HA oplog flavor too: noREMOVEis persisted while the lease is live, so standby replay cannot erase early either. The residual window between the decision-time check and the durable-finalize callback is the one every existing removal path already accepts.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?
Test commands:
Test results:
New regression
MasterServiceTest.SweepDefersToActiveReadLease: put a completed object,ExistKey(grants the lease), unmount the segment, sweep. Pre-fix the record is erased immediately; post-fix it survives the lease window (reads reportOBJECT_NOT_FOUNDwhile the handles are invalid, sinceIsValid()gatesExists()), and the sweep after lease expiry removes it for good. Verified red without the gate and green with it.MasterServiceTest.UnmountSegmentKeepsSynchronousCleanupInHaModepreviously asserted the buggy contract (erase under a live lease right afterExistKey); it now asserts survival inside the lease window and synchronous cleanup once the lease lapses.Full
master_service_testsuite: 68/68.Note on CI: current main is broken by the Environ API removal (#4513 restores it), so this branch was built and tested locally with the #4513 diff applied on top; the PR diff itself contains only the three files above. Expect the build legs to stay red here until #4513 lands, same as every other open PR.
Checklist
./scripts/code_format.shAI Assistance Disclosure
Prepared with Kimi Code (implementation, regression tests, local docker build/test runs). The submitter has reviewed the diff end to end.