[Bug]: PD P2P supply and demand are computed independently, and nothing checks they match

[liranschour]

Summary

In the PD (prefill/decode disaggregated) flow over the P2P secondary tier, the
producer's supply and the consumer's demand are computed by two
independent code paths on two different engines, and nothing anywhere verifies
that they agree. The protocol depends on them agreeing. When they diverge, the
consumer's unmatched demand parks until _LOAD_TIMEOUT_S = 30.0
(vllm/v1/kv_offload/tiering/p2p/session/client.py:33) and it recomputes the
prompt.

#52808 and #53062 are two instances of this. They were found separately, have
different triggers, and were fixed (or are being fixed) separately. Six more
divergence sites exist and are unguarded. This issue names the invariant and
proposes a test mechanism that fails CI when a future change breaks it.

Line numbers for source files are against main @ 5a4c8d9924. Line numbers
for tests/v1/kv_connector/unit/offloading_connector/test_scheduler.py are
against #52912's branch (657fcc1b4), since the tests referenced below are part
of that PR and are not on main yet.

Why PD has no enforcement, and symmetric P2P does

In symmetric P2P the consumer sends a LookupMsg, the server answers from
tier lookups and pins, and only then does the consumer fetch. Supply is
derived from demand, so they cannot disagree — a warm producer is the best
case.

The PD leg has no such round trip. P2PSecondaryTierManager.lookup for a
PD consumer is:

# PD consumer (we are the decoder): all kv blocks should be on the
# prefiller side. Return HIT immediately.
return LookupResult.HIT

  — vllm/v1/kv_offload/tiering/p2p/manager.py:350

The decoder asserts by construction that every block it wants is on the
prefiller. It never checks. Meanwhile the prefiller independently decides what
to store, through a chain of filters that know nothing about what the decoder
will ask for. The invariant "producer supply ⊇ consumer demand" is load-bearing
and unenforced.

There is no cross-check today — not even an observation

I looked for any assertion, metric, counter, or log that compares the two key
sets. There is none. The P2P tier emits no metrics at all. The three closest
signals all fall short, and two of them are dead code in PD:

1. server.py:966-979 — the only place both sides of the mismatch appear
   together, as a debug log: remaining (demanded, never supplied) next to
   leftover_available (supplied, never demanded). Counts only, no keys, off by
   default.
2. server.py:372 — if req.lookup_supplied and req.demanded: warns and
   fails the fetch immediately rather than letting the peer hit the load
   timeout. lookup_supplied is only ever set by the symmetric lookup-pin path,
   so this never fires in PD — as its own comment says.
3. client.py:229 — assert all(st.probes.get(key) is True for key in keys),
   guarded by st.probed, which PD never sets. Also dead in PD.

So the two guards that would catch a mismatch are both gated on symmetric-only
state. In PD, a mismatch surfaces only as an eventual
TransferDoneMsg(success=False) or a 30s timeout, in both cases without naming
which keys were missing. #52808 shows the practical cost: the timeout path
prints a PYTHONHASHSEED hint that had nothing to do with the failure, and the
reporter had to verify and rule it out.

The divergence surface

Each row alone is sufficient to break a round. Producer-side rows drop keys the
consumer will demand; consumer-side rows demand keys the producer will not supply.

#	Side	What drops / over-demands	Location
1	producer	if block_id == 0: continue — null-block sentinel	offloading/scheduler.py:1304
2	producer	SWA chunk unreachable within its alignment segment	scheduler.py:1312 (is_store_reachable_swa_chunk, :129)
3	producer	EAGLE/MTP volatile trailing chunk while decoding	scheduler.py:404 (in storable_chunks, :382)
4	producer	prepare_store returns None (primary allocation failure)	scheduler.py:1326
5	producer	prefix-hit key that is not HIT in primary at cascade time	tiering/manager.py:615 — #53062
6	producer	max_offload_tokens / offload_prompt_only truncation	scheduler.py:588
7	consumer	_sliding_window_lookup promotes every key it scans, not just the final window, so out-of-window chunks enter the FetchMsg	scheduler.py:633 + tiering/manager.py:404
8	consumer	eager unconditional HIT, no existence check	p2p/manager.py:350

Plus the policy gate that caused #52808: under BLOCK_LEVEL,
scheduler.py:1075 sets next_stored_chunk_idx = num_chunks, skipping
prefix-hit chunks entirely. Fixed for the producer leg by #52912.

Only #5 and the policy gate are being addressed. Rows 1, 2, 3 are currently
consistent with the demand side, but only because two hand-mirrored pieces of
logic happen to agree — which is exactly the fragility below.

Why this needs a mechanism rather than another point fix

#51840 changed lookup to return HIT_PENDING instead of RETRY when a
promotion is initiated. In _sliding_window_lookup, HIT_PENDING counts toward
the consecutive-hit streak while RETRY resets it. So a change to lookup
semantics, in a different file, silently changed which keys the consumer
demands — widening it on v0.27.1 to include 55 out-of-window SWA chunks a
warm producer can never supply. Nothing in the test suite noticed; the divergence
was found by hand-instrumenting a container and counting chunks
(#52912 (comment)).

That is the regression class to guard: the invariant spans two engines and half a
dozen files, so any single-file change can break it invisibly.

Proposed guard: exhaustiveness sentinels

The suite already has the right pattern. _SCAN_BEHAVIOR in
tests/v1/kv_connector/unit/offloading_connector/test_scheduler.py:1252 is a
table keyed by every LookupResult, with
test_scan_behavior_declared_for_every_lookup_result parametrized over
list(LookupResult) — a new enum member fails the suite until someone declares
its behavior. That is what makes a guard survive future commits instead of
pinning today's bugs.

Three sentinels in the same style would cover the surface above:

1. _CASCADE_SUPPLY_BEHAVIOR: dict[LookupResult, Disposition] — guards row 5.
For each LookupResult the primary tier can return at cascade time, declare
SUPPLY / DEFER / DROP. Drive a real TieringOffloadingManager with a
stubbed primary returning the parametrized result plus a recording request-level
tier, and assert the observed disposition matches the declared one. Today:
HIT → SUPPLY, HIT_PENDING / RETRY / MISS → DROP. When #53062 lands,
HIT_PENDING flips to DEFER and the table records that as deliberate intent
rather than a coincidence. Reuses MetricsSecondaryTierManager
(tests/v1/kv_offload/tiering/test_tiering_offloading.py:99) and the existing
manager_setup fixture.

2. _POLICY_KEEPS_PREFIX_HITS: dict[OffloadPolicy, bool] — guards the #52808
gate at scheduler.py:1075. For each OffloadPolicy, declare whether prefix-hit
chunks stay in the store path; drive request_runner over a warm prefix and
assert. A future policy has to declare, and if it declares False, assert the
P2P tier does not select it for a producer leg.

3. _GROUP_STORE_PRUNING: dict[type[KVCacheSpec], ...] — guards rows 1, 2, 3
and consumer over-demand 7. get_sliding_window_size_in_chunks
(scheduler.py:109) already switches on spec type — SlidingWindowSpec,
ChunkedLocalAttentionSpec, MambaSpec, FullAttentionSpec — and closes with
assert isinstance(kv_cache_spec, FullAttentionSpec). So a new attention type
does fail closed, but only at runtime on a model that uses it, never in CI. The
sentinel enumerates the handled spec types, requires each to declare whether the
store path prunes and which demand scan the load path uses, and for every
pruning type asserts the relation that
test_sliding_window_demand_is_store_reachable (test_scheduler.py:1517)
already checks for SWA.

These are pure-CPU unit tests needing no new harness.

What already exists, and where it stops

#52912 added test_request_level_supply_covers_consumer_demand
(test_scheduler.py:2365), which asserts the invariant at the scheduler level
over warmth × with_swa_group. Two limits worth recording:

• Supply is measured at prepare_store call args against a MagicMock
  manager, so everything downstream — the cascade's HIT-only filter (row 5),
  submit_store, and the session parking in _OutboundRequestState — is
  invisible to it. [Bugfix][KVOffload] Request-level cascade silently drops HIT_PENDING keys #53062 lives entirely in that stretch.
• Demand comes from _demanded_keys (test_scheduler.py:2343), which
  reimplements the scan on a bypass scheduler. It can drift along with
  production rather than pinning it — the [Bugfix][TieredOffloading] : Return HIT_PENDING when KV promotion is triggered #51840 failure mode.

Also worth doing, not proposed here

Listing these so they are not lost, in rough order of value:

• Tier-level supply invariant — the same assertion with a real
  TieringOffloadingManager, so the cascade filter is in scope.
• Demand-oracle drift guard — pin _demanded_keys against a real consumer
  leg's actual lookups, so the oracle cannot drift silently.
• Make the runtime failure loud — promote the server.py:966-979 debug log
  and name the unmatched keys with their KV cache group indices
  (get_offload_group_idx), so a mismatch is diagnosable from a single log line
  instead of an instrumented rebuild, and so the misleading PYTHONHASHSEED
  hint stops being the headline.
• Demand-driven serving for the PD leg, as symmetric P2P already has. This
  removes the invariant rather than guarding it, and was raised as the long-term
  direction in the [Bugfix][KVOffload] P2P tier declares REQUEST_LEVEL on the producer leg #52912 discussion (cc @Etelis @nilig @ronensc @orozery).

Happy to hand over

I have the sentinel tests locally and am glad to open the PR, or to hand the
patch to whoever is already in this code — @almogtavor and @AbarnaaSree both
volunteered on #53062, and the two overlap.

---

AI assistance was used to survey the code paths and draft this issue; the
analysis and every cited location were reviewed and verified by hand.

Before submitting a new issue...

• Make sure you already searched for relevant issues, and asked the core devs or team in the vLLM Forum first

[almogtavor]

@liranschour thanks!! I think it's better that I'll take this since I'm already working on #53062. I'll investigate this today and push the resolving PR tomorrow.

[liranschour]

Update: revising the proposed guard

@almogtavor implemented sentinels #2 and #3 in #53330, faithfully to what I
specified above. Having seen them in code I want to revise the proposal, because
one of the three does not do what I claimed it would, and the reason generalizes.

The problem statement in this issue stands unchanged: the PD producer's supply
and the consumer's demand are computed by separate code paths on separate
engines, nothing verifies they agree, #52808 and #53062 are two instances found
weeks apart, and the divergence surface table above is still accurate. What I got
wrong is the mechanism.

The test I should have applied to my own proposal

A declare-or-fail sentinel is only worth its line count if satisfying it
requires information that is not already present in the code under test. If the
required value can be read off the implementation, the cheapest path to green
produces a correct-looking entry whether or not the behavior is correct, and the
guard degrades into a diff line.

_SCAN_BEHAVIOR, the pattern I pointed at as the model, passes that test. Its
value is (lookups_issued, end_index) over a window-1 scan, and that number is
demand width — the quantity the invariant is about. You can't derive it by
inspection, because it emerges from a streak counter interacting with the result.
Filled in by running it, the entry still states the thing that matters, and a 3
sitting among 1s is legible to a reviewer.

Applying the same test to the three I proposed:

	verdict
#1 _CASCADE_SUPPLY_BEHAVIOR	Stands. Key is a LookupResult, value is the cascade's observed disposition — a different subsystem's behavior, not derivable by inspection, and SUPPLY/DEFER/DROP is exactly what supply means. Still unimplemented, and it's the one covering the prepare_store → session-parking stretch where #53062 lives. Highest value of the three.
#2 _POLICY_KEEPS_PREFIX_HITS	Half-built rather than wrong. The value is observed behavior and is semantically the right quantity. But the load-bearing half of what I specified — "if it declares False, assert the P2P tier does not select it for a producer leg" — is the part that protects the invariant, and #53330 records the fact without enforcing the consequence.
#3 _GROUP_STORE_PRUNING	Retracted.

Why #3 doesn't work

The entry it asks for is a branch of get_sliding_window_size_in_chunks, and
that function is a pure isinstance chain (scheduler.py:112-126). So the
correct entry is fully determined by what the new class inherits from, and the
assertion compares the declaration against the same function that produced it —
the oracle is the system under test. Add FooSpec(FullAttentionSpec), CI says
"not declared", the failure message (assert None == 2) hands you the value, you
write FooSpec: FullAttentionSpec, green. Nothing in that loop involves supply or
demand, and the fact needed to judge the entry — does this attention type
actually reach back arbitrarily far? — is not present in the table, the test, or
the function.

The entries as landed demonstrate it: RSWASpec, HiddenStateCacheSpec,
SinkFullAttentionSpec and MLAAttentionSpec all map to FullAttentionSpec
automatically because they subclass it. RSWASpec is worth a second look —
its docstring says gap blocks "are evicted during each decode step", so
declaring it as no-pruning is at least a question, and the table let it through
without anyone having to ask. I haven't established whether that's a real
divergence.

Two mechanical problems on top, both of which undercut the "fails in CI rather
than at runtime" claim I made for it:

• The spec set comes from KVCacheSpec.__subclasses__(), evaluated at collection
  time, so it only contains classes whose module has been imported.
  tests/v1/test_kv_cache_spec_registry.py:70 defines a module-level
  _TrulyUnregisteredSpec(KVCacheSpec); the guard survives today only because
  tests/v1/kv_connector/ sorts before it alphabetically. That's an accident,
  and it turns into a confusing failure in an unrelated PR under xdist, --lf,
  or a directory rename.
• @register_kv_cache_spec (vllm/v1/kv_cache_spec_registry.py:176) is a
  supported extension point for out-of-tree specs, which __subclasses__()
  cannot see by construction. A registry-driven or registration-time check would
  be import-order independent and would actually cover plugins.

Also: CrossAttentionSpec and EncoderOnlyAttentionSpec are declared None,
i.e. "trips the closing assert". That records a bare AssertionError at
SchedulerOffloadConfig.from_spec as sanctioned behavior for anyone enabling
offloading on an encoder-decoder model. If we keep any version of this, those
should become an explicit unsupported-configuration error or an excluded group,
not a documented crash — and pytest.raises(AssertionError) on a bare assert
is stripped under python -O.

Revised recommendation

Land the point fixes — #52912 and #53329 — since they close the two known bugs
and are independently reviewable. Hold #53330. Then, in rough order of value:

1. Runtime enforcement at the join. _OutboundRequestState.available vs
   .demanded is the one place both sets exist in a single process. A per-group
   counter plus a PD-side fast-fail covers every row of the divergence table at
   once, including rows nobody has enumerated and out-of-tree cases no CI table
   can see. Note server.py:372 already does this and is dead in PD, because
   lookup_supplied is set only by the symmetric lookup-pin path.
2. Relation tests instead of branch tables. Extend
   test_sliding_window_demand_is_store_reachable over spec types, so the oracle
   is the opposite code path. A new windowed spec then fails with a demanded key
   that was never stored, and no table entry can silence it. This is what Implement single_query_cached_kv_attention kernel #3
   should have been.
3. Sentinel Fix a bug in tying OPT embeddings #1, unchanged from the proposal above.
4. Complete Support tensor parallel #2 with the producer-leg selection assertion.
5. Demand-driven serving for the PD leg, which removes the invariant instead
   of guarding it. Still the right long-term answer.

One correction to the "also worth doing" list above: I listed a demand-oracle
drift guard as a follow-up, and #53330 implements it. I now think it argues for
its own deletion — it proves the real scan can be driven in a unit test, which
means test_request_level_supply_covers_consumer_demand should call the real scan
and _demanded_keys should go away. Pinning a reimplementation is strictly weaker
than not reimplementing, and the pin only exercises HIT_PENDING, while #51840's
failure mode was a change in which LookupResult came back.

The "make the runtime failure loud" item is still worth doing and is separable
from all of the above. #53330's version fires on every failed round including
aborts, which are routine, and a counter would serve better than a log line given
the P2P tier emits no metrics at all — but the instinct and the location are
right.

Apologies for the churn, @almogtavor — the weak part was my spec, not your
implementation.