Skip to content

[TransferEngine] Add a TCP session progress deadline - #3542

Merged
alogfans merged 2 commits into
kvcache-ai:mainfrom
jacklin78911-collab:codex/te-2930-tcp-progress-deadline
Aug 24, 2026
Merged

alogfans merged 2 commits into
kvcache-ai:mainfrom
jacklin78911-collab:codex/te-2930-tcp-progress-deadline

Conversation

@jacklin78911-collab

Copy link
Copy Markdown
Collaborator

Description

This PR adds a sliding inactivity/progress deadline to TCP client sessions, addressing the case where a peer accepts a request but stops making progress while transferring the payload.

The deadline is refreshed only after an Asio completion reports real protocol or payload progress. It is controlled by MC_TCP_PROGRESS_TIMEOUT_SEC and defaults to 30 seconds.

This is an inactivity/progress deadline, not an absolute transfer deadline. A slow transfer that continues making progress can run longer than MC_TCP_PROGRESS_TIMEOUT_SEC.

Once a matching progress-timeout handler executes, the timeout decision is sticky. A success I/O completion that was already ready but runs later cannot reverse the timeout into a clean successful completion.

Conversely, if real progress executes first, it advances the progress epoch, making the previously queued timer stale.

For WRITE operations, timeout failure is not reported until outstanding body I/O has quiesced, preserving the lifetime of the caller-owned source buffer. Timed-out connections are always reported as dirty and are not returned to the reusable connection pool.

Most of the diff is deterministic test coverage for timeout/I/O ordering, shutdown, and exactly-once completion behavior.

Related to #2930.

Module

  • Transfer Engine (mooncake-transfer-engine)

Type of Change

  • Bug fix
  • New feature
  • Refactor
  • Breaking change
  • Documentation update
  • Performance improvement
  • Other

How Has This Been Tested?

Deterministic coverage includes:

  • timeout wins over a queued final READ completion;
  • timeout wins over queued final WRITE/ACK completion;
  • progress wins and invalidates an old timer;
  • WRITE timeout waits for outstanding source-buffer I/O to quiesce;
  • no-progress WRITE payload failure;
  • partial-progress-then-stall failure;
  • shutdown with an armed progress deadline;
  • existing READ, WRITE, reconnect, shutdown, framing, and exactly-once tests.

Final local validation:

  • Normal tcp_write_visibility_test: 36/36 passed.
  • ASan/LSan/UBSan tcp_write_visibility_test: 36/36 passed with leak detection enabled and no sanitizer report.
  • clang-format --dry-run --Werror: passed.
  • git diff --check upstream/main..HEAD: passed.

Targeted tests were also exercised under TSan instrumentation during development. Test assertions passed, but TSan still reported existing project baseline races in Transport::Slice status access. No new report pointed to ClientSession, progress-timer ownership, progress_epoch_, or the sticky timeout state. This is not claimed as a clean TSan pass.

pre-commit was unavailable in the current local environment, so it is not claimed as passed.

Scope and limitations

This PR does not change:

  • endpoint metadata or endpoint-group retirement;
  • same-endpoint generation/nonce detection;
  • global peer-group bounds;
  • byte-based queue bounds;
  • the existing transfer-status synchronization model;
  • multi-rail behavior;
  • TENT TCP behavior.

Checklist

  • I have performed a self-review of my own code.
  • The changed C++ files pass clang-format --dry-run --Werror.
  • I have added tests to prove the changes are effective.
  • Documentation is not required for this scoped correctness fix.
  • pre-commit passed locally — unavailable in the current environment.

AI Assistance Disclosure

  • AI tools were used.

Codex assisted with implementation review, deterministic race-test design, local validation, and preparation of the PR description. The human submitter remains responsible for reviewing and understanding every changed line.

@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

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