invalidate_kv_cache() always returns True: worker methods return None, so all([]) makes the success guard dead code

Start in nemo_rl/models/generation/vllm/vllm_generation.py at invalidate_kv_cache, where the aggregation filters out None results and returns all([]) as True, then check the two worker methods in vllm_worker.py and vllm_worker_async.py that drop vLLM's bool return. Done means both paths propagate the result and an all-None case no longer reports success, verified against the guard in nemo_rl/weight_sync/vllm_remote_sparse_weight_synchronizer.py. Coordinate with open PR #3929, which overlaps this area, before starting.

---

READ THIS FIRST: the original framing is a duplicate

Open PR #3929, "feat(single-controller): pause generation during refit", already reports and fixes the headline claim. Its own description says:

make recompute_kv_cache_after_weight_updates effective for active vLLM requests by clearing their KV state during native pause; the previous post-refit prefix-cache reset could leave running-request KV untouched while reporting success

and, further down:

This fixes a pre-existing correctness gap: the former async reset_prefix_cache path did not clear KV blocks held by running requests, and its None worker results could still be reduced to success.

That is both of the draft's defects, named in prose, in a PR opened 2026-08-31. Filing the old version of this issue would have been closed as a duplicate, probably within an hour.

What survives the check is narrower and still worth reporting. #3929's diff touches single_controller.py, vllm_generation.py, interfaces.py, and tests. It deletes the invalidate_kv_cache call from _sync_weights and routes the SingleController path through pause_generation_for_refit instead. It does not touch vllm_worker.py or vllm_worker_async.py, and it does not change the aggregation inside invalidate_kv_cache. So after #3929 merges, VllmGeneration.invalidate_kv_cache() still returns True unconditionally, and three other call sites still depend on that return value.

Recommendation: file the narrowed version below, titled on the return value rather than on the SingleController path. Open with the acknowledgement of #3929 so a maintainer does not have to work out for themselves that this is the leftover piece. If you would rather not file at all, the next-best move is a comment on #3929 asking whether the worker-level fix is in scope, since the author clearly already knows about it.

Also note #3929 has sat at REVIEW_REQUIRED with no approval since 2026-09-28, 17 commits and +764/-99. It is not obviously about to land.

---

Narrowed issue body

Suggested title: VllmGeneration.invalidate_kv_cache() always returns True, so the guards that check it cannot fire

Claim

VllmGeneration.invalidate_kv_cache() returns True on every call that does not raise. Its own docstring says otherwise, and three callers read the return value to decide whether invalidation worked. One of them raises a RuntimeError on failure, and that RuntimeError is unreachable.

This is the worker-level residue of the gap that #3929 describes. #3929 fixes the SingleController path by not calling invalidate_kv_cache at all; the function and its other callers are unchanged.

Mechanism

nemo_rl/models/generation/vllm/vllm_generation.py:1660-1680:

    def invalidate_kv_cache(self) -> bool:
        """Invalidate reusable caches in vLLM (e.g., prefix/KV cache) after weight updates.

        For async_engine, calls reset_prefix_cache_async on workers. For sync, calls reset_prefix_cache.
        Returns True if all workers report success.
        """
        try:
            method_name = (
                "reset_prefix_cache_async"
                if self.cfg["vllm_cfg"]["async_engine"]
                else "reset_prefix_cache"
            )
            futures = self.worker_group.run_all_workers_single_data(
                method_name,
                run_rank_0_only_axes=["tensor_parallel", "pipeline_parallel"],
            )
            results = ray.get(futures)
            return all(result for result in results if result is not None)
        except Exception as e:
            print(f"Error invalidating vLLM caches: {e}")
            return False

Neither worker method returns anything.

nemo_rl/models/generation/vllm/vllm_worker.py:1543-1556:

    def reset_prefix_cache(self):
        """Reset the prefix cache of vLLM engine."""
        assert self.llm is not None, (
            "Attempting to reset prefix cache with either an uninitialized vLLM or non-model-owner"
        )

        if self.cfg["vllm_cfg"]["async_engine"]:
            raise RuntimeError(
                "reset_prefix_cache can only be used with async_engine=False. Use reset_prefix_cache_async instead."
            )

        self.llm.llm_engine.reset_prefix_cache()
        gc.collect()
        torch.cuda.empty_cache()

nemo_rl/models/generation/vllm/vllm_worker_async.py:2269-2282:

    async def reset_prefix_cache_async(self):
        """Async version of reset_prefix_cache."""
        assert self.llm is not None, (
            "Attempting to reset prefix cache with either an uninitialized vLLM or non-model-owner"
        )

        if not self.cfg["vllm_cfg"]["async_engine"]:
            raise RuntimeError(
                "reset_prefix_cache_async can only be used with async_engine=True. Use reset_prefix_cache instead."
            )

        await self.llm.reset_prefix_cache()
        gc.collect()
        torch.cuda.empty_cache()

vLLM's reset_prefix_cache returns a bool on both the sync and async engines. Dropping it makes every element of results None, the generator filters all of them out, and all([]) is True.

>>> results = [None, None, None]
>>> all(result for result in results if result is not None)
True

The None-filter was presumably written for backends that do not implement the hook. It happens to swallow the case where every backend fails to report.

Who reads the return value

nemo_rl/weight_sync/vllm_remote_sparse_weight_synchronizer.py:133:

            if not self._generation.invalidate_kv_cache():
                raise RuntimeError(
                    f"vLLM KV cache invalidation failed before {self._transport} "
                    "weight update."
                )

Unreachable. This is the same line #3746 is about, from the other side: #3746 says the call should be gated on the config flag, this says the check wrapped around it does nothing.

nemo_rl/algorithms/async_utils/trajectory_collector.py:1136-1144 takes the success branch every time, so the "only partially applied" warning never prints.

nemo_rl/algorithms/single_controller.py:5220 discards the value, which is harmless, and goes away under #3929.

Interaction with #4018

#4018 makes the drained-refit invalidation unconditional and flips the default of recompute_kv_cache_after_weight_updates to true. That is a strictly larger number of calls whose outcome nobody can observe. Worth fixing the return value first, or at least in the same window, so the new unconditional path has something to fail on.

Second, smaller item: no reset_running_requests

All four reset sites call vLLM with no arguments, so reset_running_requests defaults to False. vLLM's own docstring (vllm/v1/core/sched/scheduler.py:2607-2613 at the pinned vllm==0.29.0):

        Reset the KV prefix cache.

        If reset_running_requests is True, all the running requests will be
        preempted and moved to the waiting queue.
        Otherwise, this method will only reset the KV prefix cache when there
        is no running requests taking KV cache.

The string reset_running_requests does not appear anywhere in NeMo-RL, including tests, docs, examples, and recipe YAML.

On drained paths this is fine: nothing holds KV, so the default does the right thing. It matters on the remote-sparse synchronizer path, where #3746 establishes the call happens unconditionally and nothing in that path guarantees the engine is idle. Pre-#3929 it also mattered on the SingleController path, which is the case #3929 covers.

Note that passing reset_running_requests=True is not free. vLLM raises if preemption does not release every block (scheduler.py:2633-2639 at 0.29.0), and there are holders that while self.running: does not drain. See vllm-project/vllm#60744 for one such holder on current vLLM main. NeMo-RL does not drive vLLM's streaming-input generate API today, so that specific case is not reachable from here, but the raise is a real edge to handle rather than ignore.

Fix

Return the bool from both worker methods:

        result = self.llm.llm_engine.reset_prefix_cache()
        gc.collect()
        torch.cuda.empty_cache()
        return result

        result = await self.llm.reset_prefix_cache()
        gc.collect()
        torch.cuda.empty_cache()
        return result

Both need a -> bool annotation to match. If the None-filter is meant to stay for other backends, it should be narrowed to "no worker implemented the hook" rather than "no worker returned anything", e.g. raise or return False when results is non-empty and every element is None.

Open PR #3530 already adds reset_connector=True to all four sites. It does not add reset_running_requests and does not add returns, and it was last touched 2026-08-09 against vllm==0.25.1, so it would need a rebase either way.

---

Verification

Read first-hand on 2026-10-09 against NVIDIA-NeMo/RL main at commit
fc45e52999a137f932657c8f8dae1d6e98dbd675 (committed 2026-10-09T00:05:12Z,
"cp: fix(nightly): remove flaky step-20 logprob check 4592 (#4598)").

The earlier audit's HEAD, 2c2ecd2ea814d666edae2a8ce8b458403cb692b1, is no
longer main. Every reference below is re-read at fc45e529. A shallow clone was
used and deleted afterwards.

Corrections made to the previous draft

Previous draft said	Correct at fc45e529
single_controller.py:5215 calls invalidate_kv_cache	5215 is the if guard on the flag; the call is at 5220
vllm_generation.py:1660 fans the call out	1660 is the def; fan-out at 1672, aggregation at 1677
nemo_rl/experience/rollouts/trajectory_collector.py	path is nemo_rl/algorithms/async_utils/trajectory_collector.py; lines 1083 and 1136 are right
single_controller.py:5126 TODO(#2625) drain-gate	5125
max_lookahead_versions: 0 works because stale-abort empties the engine	mechanism is wrong, see below
no mention of #3929, #3746, #4018, #3530	all four are directly relevant; #3929 pre-empts the headline

Confirmed unchanged and correct: vllm_worker.py:1554, vllm_worker_async.py:2280,
vllm_worker_async.py:2295, vllm_remote_sparse_weight_synchronizer.py:133,
single_controller.py:5096 and :5115, grpo.py:285, ppo.py:147,
single_controller_utils/config.py:496, docs/guides/async-grpo.md:226,
pyproject.toml:167 ("vllm==0.29.0").

The max_lookahead_versions: 0 exception does not hold as written

The old draft said the stale-abort at the top of _sync_weights empties the
engine when sampler.max_lookahead_versions is 0, making that the one working
configuration. The abort path is single_controller.py:5122 ->
_abort_stale_inflight() at :4147, which aborts only groups for which
self._sampler.should_abort_inflight(...) is true. max_lookahead_versions
belongs to InOrderSampler (staleness_sampler.py:546), which extends
_GatedSampler (:408); neither overrides should_abort_inflight, so both
inherit BaseSampler's version at :250, which is return False. Under the
in-order sampler the abort aborts nothing, at any lookahead. The two samplers
that do override it (WindowedSampler:345, WeightFifoSampler:527) abort only
groups outside max_staleness_versions, not all of them.

Idleness at lookahead 0 is plausible for a different reason, the dispatch gate:
_promote_into_step's docstring at :2667-2669 says that at
in_order.max_lookahead_versions=0 "the next batch is not dispatched until this
step trains." But the gate is _dispatch_index >= trainer_version + gate_window
(staleness_sampler.py:439), and self._trainer_version += 1 runs at
single_controller.py:3337, before _sync_weights clears _rollout_permitted
at :5115. A dispatcher parked in wait_until_admissible becomes admissible in
that window. I did not trace the interleaving far enough to say whether it is
reachable in practice, so the narrowed issue claims no exception at all. Dropping
the claim is the honest move and costs nothing, since the issue no longer turns
on the SingleController path.

vllm==0.29.0 accepts reset_running_requests

This was the draft's largest unchecked assumption and it holds. Fetched from
vllm-project/vllm at tag v0.29.0 (published 2026-09-09):

• vllm/v1/engine/llm_engine.py:349-354:
  def reset_prefix_cache(self, reset_running_requests: bool = False, reset_connector: bool = False) -> bool
• vllm/v1/engine/async_llm.py:1036-1041:
  async def reset_prefix_cache(self, reset_running_requests: bool = False, reset_connector: bool = False) -> bool
• vllm/v1/core/sched/scheduler.py:2604-2644: the docstring quoted above, the
  while self.running: preemption loop, and the raise RuntimeError at
  :2633-2639.
• vllm/v1/engine/core.py:827-831: _reset_caches(reset_running_requests: bool = True, reset_connector: bool = True),
  which is what makes the pause_generation(mode="keep", clear_cache=True) path
  behave correctly. The old draft asserted this without reading it; it is right.

So the fix is a one-liner per site at the pinned version. Both parameters have
existed since before 0.29.0.

kv_holding_waiting does not exist in the 0.29.0 scheduler (0 occurrences), so
the holder in vllm-project/vllm#60744 is a post-0.29.0 structure.
WAITING_FOR_STREAMING_REQ does appear (7 occurrences), and NeMo-RL has no use
of vLLM's streaming-input generate API (grep for streaming_input,
resumable, add_streaming in nemo_rl/ finds one unrelated docstring in
automodel/checkpoint.py:358), so #60744 is not reachable from NeMo-RL today.

The all([]) mechanism: confirmed

run_all_workers_single_data (nemo_rl/distributed/worker_groups.py:979-1032)
appends one future per worker that passes the run_rank_0_only_axes filter, so
results is a list of one entry per DP leader. Both worker methods fall off the
end without a return, so every entry is None. The generator inside all(...)
filters on is not None and yields nothing. Verified by running it.

The result is the same whether results is empty or all-None, so the claim
does not depend on how many DP ranks there are.

Impact evidence, from shipped recipes

Two recipes in the repo turn the flag on, which the old draft did not mention:

• examples/configs/recipes/llm/ppo-qwen2.5-1.5b-gsm8k-2n8g-megatron-valuetp2sp-dynbatch-noncolocated-async-single-controller.yaml:71
  sets it true with max_lookahead_versions: 1 at :68; the parent config
  gives colocated.enabled: false and vllm_cfg.async_engine: true, and the
  generation backend resolves to vllm.
• examples/configs/recipes/llm/grpo-qwen2.5-math-1.5b-instruct-1n8g-megatron-single-controller-sync.yaml:30
  sets it true with max_lookahead_versions: 0 at :28.

Both are on the SingleController path, which is #3929's territory. For the
narrowed issue the useful part is just that the flag is in live use rather than
theoretical.

Duplicate search

gh search issues --repo NVIDIA-NeMo/RL --include-prs on
recompute_kv_cache_after_weight_updates, invalidate_kv_cache,
reset_prefix_cache, reset_running_requests, invalidate_kv_cache returns,
None worker results, reduced to success, and
all(result for result in results.

#3929 open PR, 2026-08-31, REVIEW_REQUIRED, updated 2026-09-28	the duplicate for the headline. Names both defects in prose. Diff deletes the invalidate_kv_cache call from _sync_weights and adds pause_generation_for_refit/resume_generation_after_refit to the SC lifecycle. Touches neither worker file. Does not change the aggregation.
#3746 open issue, 2026-08-21	same line, vllm_remote_sparse_weight_synchronizer.py:133, arguing the call should honor the config flag instead of invalidating unconditionally. Complementary, not duplicate: that issue is about whether to call, this is about whether the check around the call means anything. Its own line numbers (single_controller.py:1015, trajectory_collector.py:603) are already stale.
#3747 open issue, 2026-08-21	SGLang companion to #3746. sglang_weight_synchronizer.py:173 has the same guard shape but SGLangGeneration.invalidate_kv_cache (sglang_generation.py:942-959) returns a real True/False via try/except around an HTTP flush_cache, so it is not broken the same way. Out of scope.
#4018 open draft PR, 2026-09-05, updated 2026-10-08	makes the drained-refit invalidation unconditional and flips the default to true. Touches trajectory_collector.py, grpo.py, ppo.py, docs, tests. No worker files. Raises the stakes on the return value.
#3530 open PR, 2026-08-09	the only open PR touching both worker files. Adds reset_connector=True to all four sites and nothing else; no returns, no reset_running_requests, written against vllm==0.25.1.
#3152 merged PR, 2026-07-10	"fix(async): Always honor recompute_kv_cache_after_weight_updates". Historical context for how the flag got its current shape.
#2625 open issue, 2026-05-29	"[Single Controller / Async RL] cleanup tracking issue", the target of the TODO(#2625) comments. A tracking issue, not a duplicate, and a plausible place to link a new issue from.

GitHub issue search matches titles, bodies, and comments but not diffs, so an
unlabelled fix inside an unrelated PR would not surface. The check that settles
"not fixed" is that the defective function bodies are present verbatim at
fc45e529, which they are.

Not executed

Nothing was run except the all([]) snippet. No NeMo-RL training, no GPU, no
vLLM engine. The missing return statements and the aggregation are syntactic
facts and all([]) is a language guarantee, so the central claim needs no run.
Everything about which requests hold KV during a refit is read off the call graph
and the sampler code, which is why the narrowed issue does not assert it.