Skip to content

[Bugfix][TENT] Preserve ownership after failed memory free - #3582

Merged
alogfans merged 2 commits into
kvcache-ai:mainfrom
982945902:codex/20260821-preserve-free-ownership
Sep 1, 2026
Merged

alogfans merged 2 commits into
kvcache-ai:mainfrom
982945902:codex/20260821-preserve-free-ownership

Conversation

@982945902

@982945902 982945902 commented Aug 21, 2026 •

Copy link
Copy Markdown
Contributor

Description

TransferEngineImpl::freeLocalMemory() removed an allocation from allocated_memory_ even when the owning transport returned an error. The memory could remain allocated while the engine permanently lost the ownership record, so callers could not retry the release.

This change commits the ownership removal only after the transport reports a successful free. A fail-once transport regression test verifies that the first error preserves the allocation, the second call reaches the transport and succeeds, and a later call is correctly rejected after ownership has been released.

Module

  • Transfer Engine (mooncake-transfer-engine)

Type of Change

  • Bug fix

How Has This Been Tested?

Test commands:

cmake --build /workspaces/build --target \
  tent_local_memory_lifecycle_test tent_runtime_queue_dispatch_test -j8
/workspaces/build/tests/tent_local_memory_lifecycle_test
/workspaces/build/tests/tent_runtime_queue_dispatch_test

Test results:

  • Unit tests pass
  • Integration tests pass (if applicable)
  • Manual testing done: before the fix, a fail-once transport was called only once and the retry returned Address region not registered; the regression test now observes two transport free calls and a successful retry.

Checklist

  • I have performed a self-review of my own code
  • I have formatted my code using ./scripts/code_format.sh
  • I have run pre-commit on the files changed in this PR and all hooks pass
  • I have updated the documentation (if applicable)
  • I have added tests to prove my changes are effective
  • For changes >500 LOC: I have filed an RFC issue

pre-commit is not installed in the available development image; git diff --check and the PR-scoped C++ formatting script pass.

AI Assistance Disclosure

  • AI tools were used (specify below)

Codex helped identify and reproduce the ownership-loss path, implement the success-only ownership commit, and add the fail-once regression test. The human submitter should review and understand every changed line before merge.

++it) {
if (it->addr == addr) {
auto status = it->transport->freeLocalMemory(addr, it->size);
if (!status.ok()) return status;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Advise to prints warning.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a warning before returning the transport error. It includes the address, allocation size, and original status while preserving the ownership entry for a later retry. Fixed in ce87244.

@alogfans alogfans left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@alogfans
alogfans merged commit 6bfdb18 into kvcache-ai:main Sep 1, 2026
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants