[GH-ISSUE #2116] [Feature Request]:Make getTransferStatus a pure status query and move failover/retransmit to an explicit progress API

bsakaguchi commented

2026-10-29 21:16:43 +00:00

Owner

Originally created by @KMSorSMS on GitHub (May 16, 2026).
Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/2116

Originally assigned to: @alogfans on GitHub.

Describe your feature request

Background

While reviewing #2055, we discussed a broader API design issue around
TransferEngineImpl::getTransferStatus().

Today, getTransferStatus() is not a pure status query. In addition to
polling the underlying transport status, it may also trigger failover and
resubmit transfer tasks internally:

if (task_status.s == FAILED &&
    resubmitTransferTask(batch, task_id).ok()) {
    task.status = PENDING;
    task_status.s = PENDING;
}

This behavior predates #2055 and is not introduced by that PR. However, it
creates an implicit side effect in what appears to be an observation API.

Problem

From an integrator's perspective, especially for mooncake-pg, this implicit
behavior makes it harder to reason about transfer lifecycle ownership.

When upper layers call getTransferStatus(), they may expect it to only observe
current task or batch state. Instead, the call can also:

resubmit failed tasks,
change task transport selection,
create new in-flight transfer work,
change a task from FAILED back to PENDING,
delay when a batch becomes terminal,
affect when it is safe to call freeBatch().

This is surprising because a polling/status API can indirectly perform
communication work.

We have seen this become problematic in mooncake-pg because PG polls
getTransferStatus() from the upper layer. With the current behavior, every poll
can implicitly drive failover/retransmission inside the transfer engine, without
the PG layer being aware that new communication work has been submitted.

This makes it hard for PG to reason about transfer ownership and retry behavior:
the upper layer believes it is only observing status, while the lower layer may
be repeatedly issuing retransmits/resubmissions as a side effect of polling.

One related warning we have printed in PG when cleanup later becomes difficult
was:

LOG(WARNING)
    << "BatchID leaked due to freeBatchID "
       "failure (likely caused by a timeout): ";

The aggregation fix in #2055 helps with one part of the lifecycle issue by
preventing a batch from being reported as failed while other tasks are still
pending. However, the broader API boundary issue remains: polling status can
still implicitly advance failover and resubmission, potentially multiple times,
without the upper layer explicitly scheduling those retries.

Design Concern

Conceptually, communication operations should be explicit and scheduled by the
upper layer or by a clearly defined progress API.

A method named getTransferStatus() is expected to be observational. If it can
also mutate state and resubmit work, callers need to understand hidden lifecycle
transitions that are not obvious from the API boundary.

This coupling can be especially problematic for integrations that have their
own scheduling, timeout, cancellation, or cleanup logic.

Possible Short-Term Solution

Add a configuration flag to disable automatic failover during status polling,
for example:

disable_auto_failover_on_poll

or:

enable_auto_failover_on_poll

With this option disabled/enabled appropriately, mooncake-pg and similar
integrations could opt out of implicit resubmission from getTransferStatus().

This would be a minimal change and could provide a near-term unblock without
requiring a larger API redesign.

Expected behavior with auto failover disabled:

getTransferStatus() only returns the observed transport/task state.
A failed task remains failed.
No implicit resubmitTransferTask() is triggered from polling.
The upper layer can decide whether and when to retry, fail over, cancel, or
free the batch.

Possible Long-Term Solution

Split the current behavior into two explicit APIs:

getTransferStatus(...)

Pure observation only. It should query and return current status, but should not
resubmit, fail over, or otherwise create new communication work.

progressBatch(...) / advanceFailover(...) / progressTransfer(...)

Explicit progress API. This would drive failover, retry, resubmission, and other
state transitions for users who want the transfer engine to manage progress.

This separation would make ownership clearer:

upper layers that want explicit control can call only getTransferStatus(),
upper layers that want engine-managed failover can call the progress API,
cleanup behavior becomes easier to reason about,
freeBatch() safety becomes less dependent on hidden side effects from
polling.

Open Questions

Should automatic failover on polling remain the default for backward
compatibility?
What should the config option be named?
Should the config apply to both task-level and batch-level
getTransferStatus()?
Should getTransferStatus(batch_id, vector<TransferStatus>&) also be made
pure?
What is the exact contract for when it is safe to call freeBatch() after
failure, timeout, or cancellation?
Should failover progress be driven synchronously by an explicit API call, or
by a background progress mechanism?

Suggested Next Step

For the short term, consider adding a config flag to let integrators opt out of
implicit failover during status polling.

For the long term, discuss and design a cleaner API split where status
observation and transfer progress/failover are separate operations.

Before submitting a new issue...

Make sure you already searched for relevant issues and read the documentation

Originally created by @KMSorSMS on GitHub (May 16, 2026). Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/2116 Originally assigned to: @alogfans on GitHub. ### Describe your feature request ## Background While reviewing #2055, we discussed a broader API design issue around `TransferEngineImpl::getTransferStatus()`. Today, `getTransferStatus()` is not a pure status query. In addition to polling the underlying transport status, it may also trigger failover and resubmit transfer tasks internally: ```cpp if (task_status.s == FAILED && resubmitTransferTask(batch, task_id).ok()) { task.status = PENDING; task_status.s = PENDING; } ``` This behavior predates #2055 and is not introduced by that PR. However, it creates an implicit side effect in what appears to be an observation API. ## Problem From an integrator's perspective, especially for `mooncake-pg`, this implicit behavior makes it harder to reason about transfer lifecycle ownership. When upper layers call `getTransferStatus()`, they may expect it to only observe current task or batch state. Instead, the call can also: - resubmit failed tasks, - change task transport selection, - create new in-flight transfer work, - change a task from `FAILED` back to `PENDING`, - delay when a batch becomes terminal, - affect when it is safe to call `freeBatch()`. This is surprising because a polling/status API can indirectly perform communication work. We have seen this become problematic in `mooncake-pg` because PG polls `getTransferStatus()` from the upper layer. With the current behavior, every poll can implicitly drive failover/retransmission inside the transfer engine, without the PG layer being aware that new communication work has been submitted. This makes it hard for PG to reason about transfer ownership and retry behavior: the upper layer believes it is only observing status, while the lower layer may be repeatedly issuing retransmits/resubmissions as a side effect of polling. One related warning we have printed in PG when cleanup later becomes difficult was: ```cpp LOG(WARNING) << "BatchID leaked due to freeBatchID " "failure (likely caused by a timeout): "; ``` The aggregation fix in #2055 helps with one part of the lifecycle issue by preventing a batch from being reported as failed while other tasks are still pending. However, the broader API boundary issue remains: polling status can still implicitly advance failover and resubmission, potentially multiple times, without the upper layer explicitly scheduling those retries. ## Design Concern Conceptually, communication operations should be explicit and scheduled by the upper layer or by a clearly defined progress API. A method named `getTransferStatus()` is expected to be observational. If it can also mutate state and resubmit work, callers need to understand hidden lifecycle transitions that are not obvious from the API boundary. This coupling can be especially problematic for integrations that have their own scheduling, timeout, cancellation, or cleanup logic. ## Possible Short-Term Solution Add a configuration flag to disable automatic failover during status polling, for example: ```text disable_auto_failover_on_poll ``` or: ```text enable_auto_failover_on_poll ``` With this option disabled/enabled appropriately, `mooncake-pg` and similar integrations could opt out of implicit resubmission from `getTransferStatus()`. This would be a minimal change and could provide a near-term unblock without requiring a larger API redesign. Expected behavior with auto failover disabled: - `getTransferStatus()` only returns the observed transport/task state. - A failed task remains failed. - No implicit `resubmitTransferTask()` is triggered from polling. - The upper layer can decide whether and when to retry, fail over, cancel, or free the batch. ## Possible Long-Term Solution Split the current behavior into two explicit APIs: ```cpp getTransferStatus(...) ``` Pure observation only. It should query and return current status, but should not resubmit, fail over, or otherwise create new communication work. ```cpp progressBatch(...) / advanceFailover(...) / progressTransfer(...) ``` Explicit progress API. This would drive failover, retry, resubmission, and other state transitions for users who want the transfer engine to manage progress. This separation would make ownership clearer: - upper layers that want explicit control can call only `getTransferStatus()`, - upper layers that want engine-managed failover can call the progress API, - cleanup behavior becomes easier to reason about, - `freeBatch()` safety becomes less dependent on hidden side effects from polling. ## Open Questions 1. Should automatic failover on polling remain the default for backward compatibility? 2. What should the config option be named? 3. Should the config apply to both task-level and batch-level `getTransferStatus()`? 4. Should `getTransferStatus(batch_id, vector<TransferStatus>&)` also be made pure? 5. What is the exact contract for when it is safe to call `freeBatch()` after failure, timeout, or cancellation? 6. Should failover progress be driven synchronously by an explicit API call, or by a background progress mechanism? ## Suggested Next Step For the short term, consider adding a config flag to let integrators opt out of implicit failover during status polling. For the long term, discuss and design a cleaner API split where status observation and transfer progress/failover are separate operations. ### Before submitting a new issue... - [ ] Make sure you already searched for relevant issues and read the [documentation](https://kvcache-ai.github.io/Mooncake/)

bsakaguchi

2026-10-29 21:16:43 +00:00

closed this issue
added the
stale

auto-closed
labels

bsakaguchi commented

2026-10-29 21:16:44 +00:00

Author

Owner

@ZhenyuePan commented on GitHub (May 17, 2026):

Hi maintainers, I’d like to work on the short-term config-flag slice for this issue.

My proposed first PR would keep the existing behavior as the default for backward compatibility, and add an opt-out flag for integrations that need getTransferStatus() to be observational only.

Planned scope:

Add a TENT config option, tentatively named enable_auto_failover_on_poll, defaulting to true.
When the option is false, TransferEngineImpl::getTransferStatus() will not call resubmitTransferTask() while polling.
Apply the flag consistently to task-level and batch-level status queries.
Keep public APIs unchanged.
Add/adjust failover tests to cover:
- default behavior still auto-resubmits on poll;
- disabled behavior returns FAILED without touching the fallback transport;
- batch status aggregation remains correct when auto failover is disabled.

Before I start coding, I’d like to confirm two details:

Is enable_auto_failover_on_poll the preferred config name, or would you prefer disable_auto_failover_on_poll?
Should the flag apply to all getTransferStatus() overloads, including the vector and overall batch-status variants?

If this direction looks right, I can send a focused PR for the config-gated behavior first, leaving the larger explicit progressBatch() / advanceFailover() API split as a follow-up design discussion.

@ZhenyuePan commented on GitHub (May 17, 2026): Hi maintainers, I’d like to work on the short-term config-flag slice for this issue. My proposed first PR would keep the existing behavior as the default for backward compatibility, and add an opt-out flag for integrations that need `getTransferStatus()` to be observational only. Planned scope: 1. Add a TENT config option, tentatively named `enable_auto_failover_on_poll`, defaulting to `true`. 2. When the option is `false`, `TransferEngineImpl::getTransferStatus()` will not call `resubmitTransferTask()` while polling. 3. Apply the flag consistently to task-level and batch-level status queries. 4. Keep public APIs unchanged. 5. Add/adjust failover tests to cover: - default behavior still auto-resubmits on poll; - disabled behavior returns `FAILED` without touching the fallback transport; - batch status aggregation remains correct when auto failover is disabled. Before I start coding, I’d like to confirm two details: - Is `enable_auto_failover_on_poll` the preferred config name, or would you prefer `disable_auto_failover_on_poll`? - Should the flag apply to all `getTransferStatus()` overloads, including the vector and overall batch-status variants? If this direction looks right, I can send a focused PR for the config-gated behavior first, leaving the larger explicit `progressBatch()` / `advanceFailover()` API split as a follow-up design discussion.

bsakaguchi commented

2026-10-29 21:16:44 +00:00

Author

Owner

@KMSorSMS commented on GitHub (May 17, 2026):

Thanks for taking this on.

enable_auto_failover_on_poll sounds good to me. Since the default will be true, this name also makes the backward-compatible behavior clear.

For the scope, I think the flag should apply to all getTransferStatus() overloads, including the vector and overall batch-status variants. These APIs are all query-like from the caller’s perspective, so it would be better to keep their behavior consistent. Please just make sure the implementation is unified and clean, rather than adding separate ad-hoc checks in each path.

The focused PR for the config-gated behavior sounds good.

@KMSorSMS commented on GitHub (May 17, 2026): Thanks for taking this on. `enable_auto_failover_on_poll` sounds good to me. Since the default will be `true`, this name also makes the backward-compatible behavior clear. For the scope, I think the flag should apply to all `getTransferStatus()` overloads, including the vector and overall batch-status variants. These APIs are all query-like from the caller’s perspective, so it would be better to keep their behavior consistent. Please just make sure the implementation is unified and clean, rather than adding separate ad-hoc checks in each path. The focused PR for the config-gated behavior sounds good.

bsakaguchi commented

2026-10-29 21:16:44 +00:00

Author

Owner

@ZhenyuePan commented on GitHub (May 17, 2026):

@KMSorSMS Thanks for taking a look.

#2122 This PR is intended to cover only the short-term config-gated behavior discussed in #2116:

enable_auto_failover_on_poll defaults to true to preserve existing behavior.
When set to false, status polling no longer triggers resubmitTransferTask().
The flag is applied consistently to task-level, vector, and overall batch-status getTransferStatus() paths through a shared helper.
The larger explicit progress API split is intentionally left for follow-up discussion.

Local validation:

bash /tmp/mooncake_code_format_check.sh --check -b upstream/main
cmake --build build-tent-2116-system --target tent_failover_test tent_engine_failover_e2e_test
ctest --test-dir build-tent-2116-system -R tent_.*failover.*test --output-on-failure

I’ll keep an eye on the remaining CI jobs and follow up if anything fails.

@ZhenyuePan commented on GitHub (May 17, 2026): @KMSorSMS Thanks for taking a look. #2122 This PR is intended to cover only the short-term config-gated behavior discussed in #2116: - `enable_auto_failover_on_poll` defaults to `true` to preserve existing behavior. - When set to `false`, status polling no longer triggers `resubmitTransferTask()`. - The flag is applied consistently to task-level, vector, and overall batch-status `getTransferStatus()` paths through a shared helper. - The larger explicit progress API split is intentionally left for follow-up discussion. Local validation: - `bash /tmp/mooncake_code_format_check.sh --check -b upstream/main` - `cmake --build build-tent-2116-system --target tent_failover_test tent_engine_failover_e2e_test` - `ctest --test-dir build-tent-2116-system -R tent_.*failover.*test --output-on-failure` I’ll keep an eye on the remaining CI jobs and follow up if anything fails.

bsakaguchi commented

2026-10-29 21:16:44 +00:00

Author

Owner

@ZhenyuePan commented on GitHub (May 17, 2026):

@KMSorSMS I’m thinking about a follow-up PR after #2122 to address the longer-term API split in this issue.

My proposed direction is:

Make getTransferStatus() observation-only
- Apply this to all overloads:
  - task-level status
  - vector task-status list
  - overall batch status
- getTransferStatus() may still poll/refresh observed transport state and aggregate status, but it should not create new communication work.
- In particular, it should not call resubmitTransferTask(), change transport selection, or turn a returned FAILED status back into PENDING.
Add an explicit progress API for engine-managed failover/retry
- Proposed name: progressBatch(BatchID batch_id, TransferStatus& overall_status).
- I prefer progressBatch because the current scheduling unit is BatchID, but I’m happy to use progressTransfer or another name if maintainers prefer.
- This API would perform one explicit progress step:
  - poll pending tasks,
  - update task status,
  - trigger failover/resubmission for failed tasks when allowed,
  - aggregate and return the current overall batch status.
- If failover resubmits a task successfully, the returned/recorded status remains PENDING, preserving the current engine-managed retry behavior but moving it behind an explicit call.
Preserve existing internal wait/sync behavior
- Internal APIs such as waitTransferCompletion() and staging-buffer synchronous transfer paths can call the new progress API instead of relying on side effects from getTransferStatus().
- This keeps users who want engine-managed failover able to get the old behavior by calling the explicit progress API or existing wait/sync helpers.
Clarify cleanup semantics
- If callers only use getTransferStatus(), a terminal overall status means no hidden resubmission was scheduled by polling.
- If callers use progressBatch(), they should only call freeBatch() once the returned overall status is terminal.
- A failed individual task with other pending work should still keep the overall batch status PENDING.
API exposure
- I would start with the TENT C++ API.
- If this direction looks right, I can also expose the same operation through the TENT C API and Python binding in the same PR, so integrations do not need to depend on C++ only.
Compatibility with #2122
- #2122 adds enable_auto_failover_on_poll as a short-term compatibility/config escape hatch.
- For the long-term PR, I see two possible migration choices:
  - make getTransferStatus() pure unconditionally and keep the config only as a deprecated/transitional option, or
  - first add progressBatch() and keep the config behavior for one release, then flip/remove poll-time failover later.
- My preference is the cleaner split: getTransferStatus() pure, progressBatch() explicit.

Test coverage I plan to add/adjust:

getTransferStatus() default path no longer submits to fallback transport after observed failure.
task-level, vector, and overall status queries all remain observation-only.
progressBatch() triggers failover/resubmission and preserves the old engine-managed behavior.
waitTransferCompletion() / sync paths still progress failed transfers through the explicit progress path.
C/Python binding tests if the new API is exposed there.

Does this API split and migration direction sound right? In particular, I’d like to confirm:

preferred API name: progressBatch, progressTransfer, or something else;
whether the first long-term PR should expose C/Python bindings immediately;
whether maintainers are comfortable making getTransferStatus() pure in that PR, or prefer a two-step migration.

If maintainers prefer a separate RFC issue for the public API split, I’m happy to open one and move the detailed design there.

@ZhenyuePan commented on GitHub (May 17, 2026): @KMSorSMS I’m thinking about a follow-up PR after #2122 to address the longer-term API split in this issue. My proposed direction is: 1. Make `getTransferStatus()` observation-only - Apply this to all overloads: - task-level status - vector task-status list - overall batch status - `getTransferStatus()` may still poll/refresh observed transport state and aggregate status, but it should not create new communication work. - In particular, it should not call `resubmitTransferTask()`, change transport selection, or turn a returned `FAILED` status back into `PENDING`. 2. Add an explicit progress API for engine-managed failover/retry - Proposed name: `progressBatch(BatchID batch_id, TransferStatus& overall_status)`. - I prefer `progressBatch` because the current scheduling unit is `BatchID`, but I’m happy to use `progressTransfer` or another name if maintainers prefer. - This API would perform one explicit progress step: - poll pending tasks, - update task status, - trigger failover/resubmission for failed tasks when allowed, - aggregate and return the current overall batch status. - If failover resubmits a task successfully, the returned/recorded status remains `PENDING`, preserving the current engine-managed retry behavior but moving it behind an explicit call. 3. Preserve existing internal wait/sync behavior - Internal APIs such as `waitTransferCompletion()` and staging-buffer synchronous transfer paths can call the new progress API instead of relying on side effects from `getTransferStatus()`. - This keeps users who want engine-managed failover able to get the old behavior by calling the explicit progress API or existing wait/sync helpers. 4. Clarify cleanup semantics - If callers only use `getTransferStatus()`, a terminal overall status means no hidden resubmission was scheduled by polling. - If callers use `progressBatch()`, they should only call `freeBatch()` once the returned overall status is terminal. - A failed individual task with other pending work should still keep the overall batch status `PENDING`. 5. API exposure - I would start with the TENT C++ API. - If this direction looks right, I can also expose the same operation through the TENT C API and Python binding in the same PR, so integrations do not need to depend on C++ only. 6. Compatibility with #2122 - #2122 adds `enable_auto_failover_on_poll` as a short-term compatibility/config escape hatch. - For the long-term PR, I see two possible migration choices: - make `getTransferStatus()` pure unconditionally and keep the config only as a deprecated/transitional option, or - first add `progressBatch()` and keep the config behavior for one release, then flip/remove poll-time failover later. - My preference is the cleaner split: `getTransferStatus()` pure, `progressBatch()` explicit. Test coverage I plan to add/adjust: - `getTransferStatus()` default path no longer submits to fallback transport after observed failure. - task-level, vector, and overall status queries all remain observation-only. - `progressBatch()` triggers failover/resubmission and preserves the old engine-managed behavior. - `waitTransferCompletion()` / sync paths still progress failed transfers through the explicit progress path. - C/Python binding tests if the new API is exposed there. Does this API split and migration direction sound right? In particular, I’d like to confirm: - preferred API name: `progressBatch`, `progressTransfer`, or something else; - whether the first long-term PR should expose C/Python bindings immediately; - whether maintainers are comfortable making `getTransferStatus()` pure in that PR, or prefer a two-step migration. If maintainers prefer a separate RFC issue for the public API split, I’m happy to open one and move the detailed design there.

bsakaguchi commented

2026-10-29 21:16:44 +00:00

Author

Owner

@KMSorSMS commented on GitHub (May 18, 2026):

Thanks for writing this up. The overall API split direction sounds right to me.

I agree that getTransferStatus() should eventually become observation-only, and an explicit progress API is a cleaner place for engine-managed failover/retry. progressBatch(BatchID, TransferStatus&) also sounds reasonable to me, since the current scheduling/lifetime unit is the batch.

I also agree that internal helpers such as waitTransferCompletion() and the sync/staging paths should call the new progress API. That keeps the existing “engine-managed retry until done” behavior for blocking/sync users, while removing the side effect from status-query APIs.

For the first long-term PR, I would suggest keeping the scope focused on the TENT C++ API and the internal wait/sync paths first. If that lands cleanly and the semantics look good, we can then consider exposing the same operation through the C API and Python binding in a follow-up.

For migration, I prefer the two-step path: add progressBatch() first and keep the current config-gated compatibility behavior (in #2122) for now, then decide separately when to make getTransferStatus() pure unconditionally or deprecate/remove poll-time failover.

A few semantic details sound important to make explicit:

progressBatch() should perform one progress step, not loop until completion.
If failover resubmits a task successfully, the task/batch should remain PENDING, preserving the existing engine-managed retry behavior.
If one task fails but other tasks are still pending, the overall batch status should remain PENDING, consistent with the current aggregation behavior.

So overall: the split sounds good to me, but I’d prefer starting with a focused C++ core PR and treating bindings / full migration as follow-ups.

@KMSorSMS commented on GitHub (May 18, 2026): Thanks for writing this up. The overall API split direction sounds right to me. I agree that `getTransferStatus()` should eventually become observation-only, and an explicit progress API is a cleaner place for engine-managed failover/retry. `progressBatch(BatchID, TransferStatus&)` also sounds reasonable to me, since the current scheduling/lifetime unit is the batch. I also agree that internal helpers such as `waitTransferCompletion()` and the sync/staging paths should call the new progress API. That keeps the existing “engine-managed retry until done” behavior for blocking/sync users, while removing the side effect from status-query APIs. For the first long-term PR, I would suggest keeping the scope focused on the TENT C++ API and the internal wait/sync paths first. If that lands cleanly and the semantics look good, we can then consider exposing the same operation through the C API and Python binding in a follow-up. For migration, I prefer the two-step path: add `progressBatch()` first and keep the current config-gated compatibility behavior (in #2122) for now, then decide separately when to make `getTransferStatus()` pure unconditionally or deprecate/remove poll-time failover. A few semantic details sound important to make explicit: - `progressBatch()` should perform one progress step, not loop until completion. - If failover resubmits a task successfully, the task/batch should remain `PENDING`, preserving the existing engine-managed retry behavior. - If one task fails but other tasks are still pending, the overall batch status should remain `PENDING`, consistent with the current aggregation behavior. So overall: the split sounds good to me, but I’d prefer starting with a focused C++ core PR and treating bindings / full migration as follow-ups.

bsakaguchi commented

2026-10-29 21:16:44 +00:00

Author

Owner

@alogfans commented on GitHub (May 19, 2026):

My advise is to monitors whether an alternative transport is chosen using in a seperate thread. Within the thread, callback is more efficient than busy polling. That is, if a request fails, it post a message to the dedicated thread and submits again using another transports.

@alogfans commented on GitHub (May 19, 2026): My advise is to monitors whether an alternative transport is chosen using in a seperate thread. Within the thread, callback is more efficient than busy polling. That is, if a request fails, it post a message to the dedicated thread and submits again using another transports.

bsakaguchi commented

2026-10-29 21:16:45 +00:00

Author

Owner

@KMSorSMS commented on GitHub (May 19, 2026):

Thanks for the suggestion. I agree that if we want engine-managed automatic failover, an event/callback-driven background worker is a better direction than busy polling.

One point I want to clarify is that the main issue here is not whether polling or callback is used internally, but who owns the retry/failover process. Many integrations still need to poll getTransferStatus() to observe completion, but a status-query API should not implicitly resubmit work or switch transports. For some use cases, such as fixed transport paths, GPU-kernel-related transfers, or CUDA graph style execution, switching to another transport after a runtime failure may be incorrect. In those cases, the caller may want the transfer to fail and decide explicitly whether/when to retry.

So I think these concerns are compatible if we separate the concepts:

getTransferStatus() should remain observation-only.
progressBatch() can be the explicit caller-driven way to advance retry/failover state.
A background worker can be an optional engine-managed progress mode, ideally driven by callbacks/events instead of busy polling.

In the engine-managed mode, getTransferStatus() would still only read the engine’s public state. If a transport failure is observed internally and the background worker successfully resubmits on an allowed fallback transport, the public task/batch state can remain PENDING. It should only become terminal FAILED when failover is disabled, retry budget is exhausted, no fallback transport is available, or resubmission fails.

I also think this should ideally be controlled at the batch/request level, not only globally. Some batches may want engine-managed failover, while others may need user-managed progress or failover disabled entirely.

For the next focused PR, I still prefer starting with the C++ API split: make the status-query path observation-only and add an explicit progress API. The background worker/callback-driven engine-managed mode sounds useful, but it probably deserves a separate follow-up design because it brings in thread lifecycle, event queues, freeBatch() synchronization, and ownership/race semantics.

Does this separation sound reasonable to you? My current preference is to keep an explicit progress API for callers that need control, and treat the background worker as an optional engine-managed mode for callers that want automatic failover. I’d like to hear whether you think this split makes sense.

@KMSorSMS commented on GitHub (May 19, 2026): Thanks for the suggestion. I agree that if we want engine-managed automatic failover, an event/callback-driven background worker is a better direction than busy polling. One point I want to clarify is that the main issue here is not whether polling or callback is used internally, but who owns the retry/failover process. Many integrations still need to poll `getTransferStatus()` to observe completion, but a status-query API should not implicitly resubmit work or switch transports. For some use cases, such as fixed transport paths, GPU-kernel-related transfers, or CUDA graph style execution, switching to another transport after a runtime failure may be incorrect. In those cases, the caller may want the transfer to fail and decide explicitly whether/when to retry. So I think these concerns are compatible if we separate the concepts: - `getTransferStatus()` should remain observation-only. - `progressBatch()` can be the explicit caller-driven way to advance retry/failover state. - A background worker can be an optional engine-managed progress mode, ideally driven by callbacks/events instead of busy polling. In the engine-managed mode, `getTransferStatus()` would still only read the engine’s public state. If a transport failure is observed internally and the background worker successfully resubmits on an allowed fallback transport, the public task/batch state can remain `PENDING`. It should only become terminal `FAILED` when failover is disabled, retry budget is exhausted, no fallback transport is available, or resubmission fails. I also think this should ideally be controlled at the batch/request level, not only globally. Some batches may want engine-managed failover, while others may need user-managed progress or failover disabled entirely. For the next focused PR, I still prefer starting with the C++ API split: make the status-query path observation-only and add an explicit progress API. The background worker/callback-driven engine-managed mode sounds useful, but it probably deserves a separate follow-up design because it brings in thread lifecycle, event queues, `freeBatch()` synchronization, and ownership/race semantics. Does this separation sound reasonable to you? My current preference is to keep an explicit progress API for callers that need control, and treat the background worker as an optional engine-managed mode for callers that want automatic failover. I’d like to hear whether you think this split makes sense.

bsakaguchi commented

2026-10-29 21:16:45 +00:00

Author

Owner

@ZhenyuePan commented on GitHub (May 19, 2026):

@KMSorSMS @alogfans Thanks, this makes sense to me. I agree that an event/callback-driven worker is a better direction for engine-managed automatic failover than relying on status polling.

Based on the latest discussion, I see two related pieces here:

API ownership boundary:
- getTransferStatus() should be observation-only.
- failover/retry should be driven by an explicit progress path, for example:
```
Status progressBatch(BatchID batch_id, TransferStatus& overall_status);
```
Engine-managed progress mechanism:
- for users who want automatic failover, the engine can run a dedicated worker thread;
- failed requests can post events/messages to that worker;
- the worker can resubmit through an allowed fallback transport;
- this should be more efficient and cleaner than making ordinary status polling drive resubmission.

My proposed next step is to clarify whether these two pieces should land together or in two stages.

If maintainers prefer landing them together, I can plan the next PR around:

adding the C++ progressBatch() API as the explicit progress/failover operation;
adding an internal engine-managed failover worker/event queue;
routing automatic failover through that worker instead of getTransferStatus();
keeping getTransferStatus() as a status observation path;
preserving existing blocking/sync behavior by having waitTransferCompletion() / sync paths use the explicit progress or engine-managed path.

The main design points I would want to make explicit before coding are:

whether the worker is always enabled for engine-managed failover or controlled by config;
whether the policy should be global first or batch/request-level;
how freeBatch() synchronizes with queued failover work;
how terminal FAILED is reported when failover is disabled, retry budget is exhausted, no fallback exists, or resubmission fails;
whether transports should expose a common failure/completion event hook, or whether the first version should keep the event source inside the transfer engine.

Would you prefer the next PR to include both the progressBatch() API split and the callback/event-driven worker, or should I first land the C++ API boundary and then add the worker mechanism in a follow-up?

@ZhenyuePan commented on GitHub (May 19, 2026): @KMSorSMS @alogfans Thanks, this makes sense to me. I agree that an event/callback-driven worker is a better direction for engine-managed automatic failover than relying on status polling. Based on the latest discussion, I see two related pieces here: 1. API ownership boundary: - `getTransferStatus()` should be observation-only. - failover/retry should be driven by an explicit progress path, for example: ```cpp Status progressBatch(BatchID batch_id, TransferStatus& overall_status); ``` 2. Engine-managed progress mechanism: - for users who want automatic failover, the engine can run a dedicated worker thread; - failed requests can post events/messages to that worker; - the worker can resubmit through an allowed fallback transport; - this should be more efficient and cleaner than making ordinary status polling drive resubmission. My proposed next step is to clarify whether these two pieces should land together or in two stages. If maintainers prefer landing them together, I can plan the next PR around: - adding the C++ `progressBatch()` API as the explicit progress/failover operation; - adding an internal engine-managed failover worker/event queue; - routing automatic failover through that worker instead of `getTransferStatus()`; - keeping `getTransferStatus()` as a status observation path; - preserving existing blocking/sync behavior by having `waitTransferCompletion()` / sync paths use the explicit progress or engine-managed path. The main design points I would want to make explicit before coding are: - whether the worker is always enabled for engine-managed failover or controlled by config; - whether the policy should be global first or batch/request-level; - how `freeBatch()` synchronizes with queued failover work; - how terminal `FAILED` is reported when failover is disabled, retry budget is exhausted, no fallback exists, or resubmission fails; - whether transports should expose a common failure/completion event hook, or whether the first version should keep the event source inside the transfer engine. Would you prefer the next PR to include both the `progressBatch()` API split and the callback/event-driven worker, or should I first land the C++ API boundary and then add the worker mechanism in a follow-up?

bsakaguchi commented

2026-10-29 21:16:45 +00:00

Author

Owner

@KMSorSMS commented on GitHub (May 20, 2026):

I think the key point before introducing the engine-managed worker mechanism is backward compatibility.

With #2122, the old behavior is still preserved behind enable_auto_failover_on_poll, and the default remains true. So existing users can continue to get the current poll-time auto failover/resubmit behavior from getTransferStatus(), while integrations such as mooncake-pg can opt into observation-only polling by setting the flag to false.

Given that, I would prefer the next PR to first add the C++ API boundary / explicit progress path without removing the existing compatibility mode. In other words:

keep getTransferStatus() behavior backward-compatible by default through enable_auto_failover_on_poll = true;
allow observation-only getTransferStatus() when the flag is disabled;
add progressBatch() as the explicit progress/failover path for users that want to move away from poll-time side effects;
defer the callback/event-driven worker to a follow-up design/PR.

This way, before the engine-managed worker mechanism exists, we do not break the previous operating model. We only add another clearer mode and a migration path. Once the worker design is ready, we can decide separately whether and when to change the default or deprecate poll-time auto failover.

@KMSorSMS commented on GitHub (May 20, 2026): I think the key point before introducing the engine-managed worker mechanism is backward compatibility. With #2122, the old behavior is still preserved behind `enable_auto_failover_on_poll`, and the default remains `true`. So existing users can continue to get the current poll-time auto failover/resubmit behavior from `getTransferStatus()`, while integrations such as mooncake-pg can opt into observation-only polling by setting the flag to `false`. Given that, I would prefer the next PR to first add the C++ API boundary / explicit progress path without removing the existing compatibility mode. In other words: - keep `getTransferStatus()` behavior backward-compatible by default through `enable_auto_failover_on_poll = true`; - allow observation-only `getTransferStatus()` when the flag is disabled; - add `progressBatch()` as the explicit progress/failover path for users that want to move away from poll-time side effects; - defer the callback/event-driven worker to a follow-up design/PR. This way, before the engine-managed worker mechanism exists, we do not break the previous operating model. We only add another clearer mode and a migration path. Once the worker design is ready, we can decide separately whether and when to change the default or deprecate poll-time auto failover.

bsakaguchi commented

2026-10-29 21:16:45 +00:00

Author

Owner

@ZhenyuePan commented on GitHub (May 23, 2026):

@KMSorSMS @alogfans I've opened #2160 as the focused first step we discussed.

Scope, aligned with the consensus in this thread:

Adds an explicit Status progressBatch(BatchID, TransferStatus&) on the TENT C++ API.
progressBatch() always allows internal failover/resubmit, independent of enable_auto_failover_on_poll.
Single progress step per call, non-blocking; callers loop if they want to wait.
Failover that successfully resubmits keeps the task/batch as PENDING, preserving engine-managed retry semantics.
Partial pending → overall PENDING; terminal states are not revived.
waitTransferCompletion() and the sync/staging paths now drive progress via progressBatch() instead of relying on getTransferStatus() side effects.

Backward compatibility:

getTransferStatus() behavior is unchanged by default; the enable_auto_failover_on_poll switch from #2122 still gates poll-time auto failover.
No changes to the C API or Python binding in this PR — kept C++-only on purpose, as suggested.
No background worker / event queue — explicitly deferred to a follow-up so we can discuss thread lifecycle, freeBatch() synchronization, and per-batch policy separately.

Test coverage (7 new e2e cases in engine_failover_e2e_test.cpp):

ProgressBatchRetriesWhenPollAutoFailoverDisabled
ProgressBatchDoesNotReviveObservedFailedTask
ProgressBatchHonorsMaxFailoverAttemptsZero
ProgressBatchKeepsOverallPendingWithMixedOutcomes
ProgressBatchKeepsPendingAfterSuccessfulFailoverResubmit
TransferSyncUsesProgressBatchWhenPollDisabled
ProgressBatchAdvancesExactlyOneStepPerCall

Local validation in the project's Ubuntu 22.04 devcontainer image: 51/51 TENT failover tests pass, scripts/code_format.sh --check -b upstream/main clean.

Happy to iterate on naming or split semantics if you have further preferences before review.

@ZhenyuePan commented on GitHub (May 23, 2026): @KMSorSMS @alogfans I've opened #2160 as the focused first step we discussed. Scope, aligned with the consensus in this thread: - Adds an explicit `Status progressBatch(BatchID, TransferStatus&)` on the TENT C++ API. - `progressBatch()` always allows internal failover/resubmit, independent of `enable_auto_failover_on_poll`. - Single progress step per call, non-blocking; callers loop if they want to wait. - Failover that successfully resubmits keeps the task/batch as `PENDING`, preserving engine-managed retry semantics. - Partial pending → overall `PENDING`; terminal states are not revived. - `waitTransferCompletion()` and the sync/staging paths now drive progress via `progressBatch()` instead of relying on `getTransferStatus()` side effects. Backward compatibility: - `getTransferStatus()` behavior is unchanged by default; the `enable_auto_failover_on_poll` switch from #2122 still gates poll-time auto failover. - No changes to the C API or Python binding in this PR — kept C++-only on purpose, as suggested. - No background worker / event queue — explicitly deferred to a follow-up so we can discuss thread lifecycle, `freeBatch()` synchronization, and per-batch policy separately. Test coverage (7 new e2e cases in `engine_failover_e2e_test.cpp`): - `ProgressBatchRetriesWhenPollAutoFailoverDisabled` - `ProgressBatchDoesNotReviveObservedFailedTask` - `ProgressBatchHonorsMaxFailoverAttemptsZero` - `ProgressBatchKeepsOverallPendingWithMixedOutcomes` - `ProgressBatchKeepsPendingAfterSuccessfulFailoverResubmit` - `TransferSyncUsesProgressBatchWhenPollDisabled` - `ProgressBatchAdvancesExactlyOneStepPerCall` Local validation in the project's Ubuntu 22.04 devcontainer image: 51/51 TENT failover tests pass, `scripts/code_format.sh --check -b upstream/main` clean. Happy to iterate on naming or split semantics if you have further preferences before review.

bsakaguchi commented

2026-10-29 21:16:45 +00:00

Author

Owner

@ZhenyuePan commented on GitHub (May 26, 2026):

@KMSorSMS @alogfans Here's my planned merge sequence for the related PRs:

~~#2122 — enable_auto_failover_on_poll config flag (already merged)~~
#2160 — Explicit progressBatch() API (need to fix code formatting first, then merge)
#2199 — ProgressWorker skeleton (will update the code after #2160 is merged)
Once the skeleton looks good, let's discuss and finalize the feature together
#2208 — I'll write up the complete documentation in one go after the feature is finalized

#2159 was closed and superseded by #2208.

So the immediate next step is landing #2160. Will push the format fix shortly.

@ZhenyuePan commented on GitHub (May 26, 2026): @KMSorSMS @alogfans Here's my planned merge sequence for the related PRs: 1. ~~#2122 — `enable_auto_failover_on_poll` config flag (already merged)~~ 2. **#2160** — Explicit `progressBatch()` API (need to fix code formatting first, then merge) 3. **#2199** — ProgressWorker skeleton (will update the code after #2160 is merged) 4. Once the skeleton looks good, let's discuss and finalize the feature together 5. **#2208** — I'll write up the complete documentation in one go after the feature is finalized #2159 was closed and superseded by #2208. So the immediate next step is landing #2160. Will push the format fix shortly.

bsakaguchi commented

2026-10-29 21:16:46 +00:00

Author

Owner

@ZhenyuePan commented on GitHub (May 27, 2026):

CC @KMSorSMS @alogfans

PR #2199 ([TE] Add ProgressWorker skeleton) has been merged. #2160 (progressBatch API) was a prerequisite of #2199 and its content is already included in main — it can be closed.

Done so far:

progressBatch() explicit progress-driving API (from #2160)
Default-off ProgressWorker driven by batch-ready notifications
Engine-level batch lifetime protection (safe to race with freeBatch())
enable_progress_worker config + notifyBatchMaybeReady() test hook
Progress path serialized by progress_mutex_

Planned follow-up PRs:

PR 3: Transport terminal-event integration

Wire transport completion/failure callbacks into ProgressWorker
Transport calls notifyBatchMaybeReady() on completion, so the worker is automatically driven by transport events

PR 4: lazy-free behavior change + lifecycle fix

Adjust freeBatch() deferred-release logic: when ProgressWorker is still processing a batch, freeBatch() must wait or mark for deferred release
Remove thread-local batch_set_ dependency, making batch lifecycle fully thread-agnostic

PR 5: Pure-observation getTransferStatus() + documentation update

Change enable_auto_failover_on_poll default to false
getTransferStatus() will no longer implicitly call resubmitTransferTask() to resubmit failed tasks — becomes a pure read-only operation
Users who need failover should explicitly call progressBatch() or enable ProgressWorker
Update documentation: API change description, migration guide, configuration reference

Open for discussion on priority and scope of each step. Feedback welcome!

@ZhenyuePan commented on GitHub (May 27, 2026): CC @KMSorSMS @alogfans PR #2199 (`[TE] Add ProgressWorker skeleton`) has been merged. #2160 (`progressBatch` API) was a prerequisite of #2199 and its content is already included in main — it can be closed. **Done so far:** - `progressBatch()` explicit progress-driving API (from #2160) - Default-off `ProgressWorker` driven by batch-ready notifications - Engine-level batch lifetime protection (safe to race with `freeBatch()`) - `enable_progress_worker` config + `notifyBatchMaybeReady()` test hook - Progress path serialized by `progress_mutex_` **Planned follow-up PRs:** **PR 3: Transport terminal-event integration** - Wire transport completion/failure callbacks into `ProgressWorker` - Transport calls `notifyBatchMaybeReady()` on completion, so the worker is automatically driven by transport events **PR 4: lazy-free behavior change + lifecycle fix** - Adjust `freeBatch()` deferred-release logic: when `ProgressWorker` is still processing a batch, `freeBatch()` must wait or mark for deferred release - Remove thread-local `batch_set_` dependency, making batch lifecycle fully thread-agnostic **PR 5: Pure-observation `getTransferStatus()` + documentation update** - Change `enable_auto_failover_on_poll` default to `false` - `getTransferStatus()` will no longer implicitly call `resubmitTransferTask()` to resubmit failed tasks — becomes a pure read-only operation - Users who need failover should explicitly call `progressBatch()` or enable `ProgressWorker` - Update documentation: API change description, migration guide, configuration reference Open for discussion on priority and scope of each step. Feedback welcome!

bsakaguchi commented

2026-10-29 21:16:46 +00:00

Author

Owner

@github-actions[bot] commented on GitHub (Aug 26, 2026):

This issue has had no activity for 90 days and will be closed in 7 days if there is no further activity. Please comment or react if it should stay open.

@github-actions[bot] commented on GitHub (Aug 26, 2026): This issue has had no activity for 90 days and will be closed in 7 days if there is no further activity. Please comment or react if it should stay open.

bsakaguchi commented

2026-10-29 21:16:46 +00:00

Author

Owner

@github-actions[bot] commented on GitHub (Sep 3, 2026):

Closing due to 3 months of inactivity. If this is still relevant, please comment and we can reopen.

@github-actions[bot] commented on GitHub (Sep 3, 2026): Closing due to 3 months of inactivity. If this is still relevant, please comment and we can reopen.

bsakaguchi referenced this issue

2026-10-29 21:30:00 +00:00

[PR #690] [MERGED] [Store]feat: Migrate Persistence Metadata from Client to Master Service #1747

Rows
Columns

[GH-ISSUE #2116] [Feature Request]:Make getTransferStatus a pure status query and move failover/retransmit to an explicit progress API #578

Describe your feature request

Background

Problem

Design Concern

Possible Short-Term Solution

Possible Long-Term Solution

Open Questions

Suggested Next Step

Before submitting a new issue...