[Bugfix][TENT] Replace stale buffers when refreshing segment info - #4229
Draft
jacklin78911-collab wants to merge 1 commit into
Draft
jacklin78911-collab wants to merge 1 commit into
jacklin78911-collab wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Reusing a
SegmentInfooutput object withgetSegmentInfo()appends new buffers to the previous result. Repeated queries duplicate entries, and a refresh after unregistering a buffer still reports that old address.Clear the output buffer list after acquiring the current segment descriptor, before filling either the memory or file result. This is a one-line runtime fix. Add a regression to the existing local-memory lifecycle test that queries twice into one object, unregisters the buffer, and queries again.
This fixes output-object reuse, not the remote descriptor cache. The existing TENT CI job already runs this test target. No new state, configuration or test target is needed.
Module
mooncake-transfer-engine)Type of Change
How Has This Been Tested?
Built from main
2f13d400da4a3eb04d37263742866c1ce555d212, withUSE_TENT=ON,USE_CUDA=OFF, andBUILD_UNIT_TESTS=ON.cmake --build build --target tent_local_memory_lifecycle_test -j4 ctest --test-dir build --output-on-failure -R '^tent_local_memory_lifecycle_test$'Baseline regression fails: the second query reports two entries for one registered buffer, and querying after unregistration still returns entries.
Fixed: both local-memory lifecycle tests pass, including the existing failed-free ownership regression.
Pre-commit passes on the two changed files;
git diff --checkpasses.Unit tests pass
Manual baseline/fixed comparison done
Upstream CI on
bd3b2f645: all four TENT matrix jobs pass, including the local-memory lifecycle test suite in the CUDA-off job. The overall Build & Test run is red because the separate Storefileread_worker_pool_testfails (215/216 general CTests pass): process thread count grows beyond the test baseline, followed by the 1 s teardown-count timeout. This matches [CI] FilereadWorkerPoolTest.AcceptsTypedTrailingWhitespaceAndCaches is flaky on the 1s teardown window #4213; existing fixes [Bugfix][Store] Isolate fileread worker count assertions from runtime threads #4210 and [CI] Harden FilereadWorkerPool thread-count flake #4226 are still open. No Store test changes are included here. The author account cannot rerun upstream jobs (repository admin rights required).Checklist
./scripts/code_format.shAI Assistance Disclosure
Codex assisted with diagnosis, implementation, regression validation and the second-pass review. This is a draft for human review; no human review is claimed.