Skip to content

Commit e500666

Browse files
committed
findings: PR #49877 (fix for #49820) validated at 3 levels
Etelis's fix (round-scoped serve state + queued client fetches) validated: 1. Their own test suite run by us: 8/8 fail on baseline, 209/209 pass patched. 2. A new regression we wrote (repro_49877_finish_gap.py) surfacing one remaining gap: finish() drops the active load without a LoadResult. Empirically confirmed against their own patched code, not just traced. 3. End-to-end GPU repro on our own workload: 612->0 #49820 stalls, 13->0 duplicate-fetch disconnects, 143->12 max deferred, 9min->4m36s completion. Net: the fix genuinely works, independently verified, not just trusted from the PR description. One narrower gap remains, well-scoped with a fix direction grounded in an existing pattern in the same file. Signed-off-by: nilig <nili.ifergan@gmail.com>
1 parent ff1cc5d commit e500666

1 file changed

Lines changed: 64 additions & 13 deletions

File tree

test/p2p-findings/upstream-issues-map.md

Lines changed: 64 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ directly on GitHub.
1313
|---|---|---|---|---|---|
1414
| [#49635](https://github.com/vllm-project/vllm/issues/49635) | late KV-offload store after request finalization -> `KeyError` (`_req_state` deleted at `tiering/manager.py:721`, indexed unguarded at `:542` via `scheduler.py:1045 _build_store_jobs`) | OffloadingConnector store path -- NOT P2P-specific | crash | neither catalog | [#49671](https://github.com/vllm-project/vllm/pull/49671) **MERGED** 2026-07-25 |
1515
| [#49809](https://github.com/vllm-project/vllm/issues/49809) | reconnect to a reaped peer trips `AssertionError: ZmqConnection already exists`; dead conn never released | P2P control transport (`tiering/p2p/control/zmq.py`) | crash | pd-defects Defect 2 | [#49823](https://github.com/vllm-project/vllm/pull/49823) open, validated (5 fail stock / 16 pass patched), our review comment posted on the PR directly |
16-
| [#49820](https://github.com/vllm-project/vllm/issues/49820) | symmetric producer accepts a fetch it cannot serve, never sends `TransferDone(success=False)` -> consumer deferred full `_LOAD_TIMEOUT_S=30s`. ROOT CAUSE FOUND 2026-07-26 -- see below | P2P session (`tiering/p2p/session/*`) | stall | new (post-Liran residual) | none |
16+
| [#49820](https://github.com/vllm-project/vllm/issues/49820) | symmetric producer accepts a fetch it cannot serve, never sends `TransferDone(success=False)` -> consumer deferred full `_LOAD_TIMEOUT_S=30s`. ROOT CAUSE FOUND 2026-07-26 -- see below | P2P session (`tiering/p2p/session/*`) | stall | new (post-Liran residual) | [#49877](https://github.com/vllm-project/vllm/pull/49877) open, VALIDATED at 3 levels incl. our own GPU repro -- works end-to-end, 1 narrower gap remains -- see below |
1717
| [#49829](https://github.com/vllm-project/vllm/issues/49829) | `TieringOffloadingManager.lookup()` returns `HIT_PENDING` unconditionally, no deadline, no downgrade-to-MISS | shared tiering manager (`tiering/manager.py`) -- NOT P2P-specific | stall | lookup-hangs Defect 3 | [#49850](https://github.com/vllm-project/vllm/pull/49850) DRAFT (author flags end-to-end/scale validation as pending); we validated unit-level and posted 2 confirmed review findings -- see below |
1818
| -- | the generic/symmetric P2P secondary tier itself (peer lookup + serving via `ParentManager`) -- the tier all four bugs above exercise | P2P tier, foundational | -- | -- | [#48021](https://github.com/vllm-project/vllm/pull/48021) open, APPROVED by orozery 2026-07-22, not yet merged (contains the one-fetch contract fixing lookup-hangs Defect 1+2) |
1919

@@ -92,10 +92,58 @@ correction passes (my first draft wrongly credited `add_fetch_demand()`'s
9292
`add_stored_blocks()` reusing the live object, verified directly in code);
9393
the #49829 comment was confirmed accurate as posted, no edits needed.
9494

95-
Next step (not done): a deterministic regression test -- submit load A,
96-
submit load B same `kv_request_id` before A completes, disconnect, assert
97-
both get terminal failure, assert neither primary allocation remains
98-
`HIT_PENDING`.
95+
Superseded by PR #49877 below, which independently implements a round-scoped
96+
fix and its own regression test covering this exact scenario.
97+
98+
## PR #49877 (Etelis's fix for #49820): validated at 3 levels, works end-to-end
99+
100+
`https://github.com/vllm-project/vllm/pull/49877`, "Scope serve state to
101+
fetch rounds", opened by `Etelis` (who claimed #49820) hours after the root
102+
cause above was posted. FIX #49820 explicitly: server keeps the current
103+
fetch round and not-yet-fetched supply separate (fixes the shared
104+
`_OutboundRequestState` mechanism); client allows only one `FetchMsg` in
105+
flight per id, queuing extras instead of overwriting `st.load` (fixes the
106+
completion-misattribution mechanism); manager adds a per-request wire-id
107+
suffix for symmetric consumers. Author's own testing: 209 tests passed (10
108+
new), plus a 2xH100 DeepSeek-V2-Lite run (191/192 requests finish vs ~20/192
109+
on main).
110+
111+
**Validated at 3 levels, not just trusted from the PR description:**
112+
113+
1. **Their own test suite, run by us.** Baseline = our `combined_overlay`
114+
(exactly #48021's current head + #49823, i.e. #49877's actual base --
115+
NOT stock nightly, which is missing #48021's `LookupMsg`/`LookupRespMsg`
116+
protocol additions entirely). `test_issue_49820_repro.py`: 8/8 FAIL on
117+
baseline. Full `tests/v1/kv_offload/tiering/p2p/` with #49877's
118+
manager.py/client.py/server.py swapped in: **209/209 PASS**, exact match
119+
to the claimed count.
120+
121+
2. **A new, narrower regression we wrote and ran** (`scratchpad/
122+
repro_49877_finish_gap.py`), surfacing one remaining gap: `ClientRole.
123+
finish()` correctly fails every queued fetch, but for the ACTIVE
124+
`st.load` it sends `AbortFetchMsg`, clears `st.load`, and produces no
125+
`LoadResult`. The later `AbortAckMsg` then finds nothing to complete.
126+
`P2PSecondaryTierManager.on_request_finished()` has no alternate
127+
completion path. Result against #49877's own patched code: zero
128+
`LoadResult`s produced -- empirically confirmed, not just traced.
129+
Fix direction (grounded in an existing pattern in the same file): don't
130+
clear `st.load` immediately in `finish()`; arm `aborted_at` and let the
131+
same abort-ack/timeout mechanism used elsewhere resolve it. Posted as a
132+
review comment on the PR.
133+
134+
3. **End-to-end GPU validation, our own workload** (not Etelis's harness).
135+
Redeployed the same uc2-llama 4-pod rig, same nightly, same hi-rate
136+
profile as the #49820 repro above, with #49877's patched manager/client/
137+
server. Result: harness completed both load stages in 4m36s (was 9+ min,
138+
manually stopped, before); 0 restarts; 0 crash signatures; **0** `#49820`
139+
stall signatures (was 612); **0** duplicate-fetch disconnects (was 13);
140+
pull mechanism genuinely engaged (1193 fetch RECEIVED, 262 write_blocks,
141+
131 transfer_done); **0** collapse snapshots (was frequent); max
142+
concurrent deferred **12** (was up to 143). Logs: `scratchpad/dbg49877/`.
143+
144+
**Net:** the fix genuinely works, independently verified at three levels.
145+
One narrower, empirically-confirmed gap remains (`finish()` on an active
146+
load), well-scoped with a sound fix direction.
99147

100148
## PR #49850 (fix for #49829): architecture + open findings
101149

@@ -150,14 +198,17 @@ Two catalogs both start at "Defect 1", causing collisions:
150198
One crash resolved (#49635, via merged #49671); one crash has an open,
151199
validated fix PR pending merge (#49809 -> #49823, our review comment posted
152200
directly on the PR); lookup-hangs Defect 1+2 covered by #48021 (open,
153-
APPROVED, not yet merged). Both remaining stalls now have a CONFIRMED root
154-
cause (see "Root causes resolved" above), tracing to the same design gap --
155-
no round/generation identifier for multi-round P2P promotions under one
156-
`kv_request_id`. #49829 additionally has an open draft fix PR with 2
157-
unresolved correctness findings (#49850). #49820 has no fix PR yet. Until
158-
#49850's findings are addressed and it un-drafts, and until #49820 gets a fix
159-
PR (or the shared root cause is fixed once, which would likely close both),
160-
these two remain the current load-readiness blockers.
201+
APPROVED, not yet merged). Both remaining stalls trace to the same design
202+
gap -- no round/generation identifier for multi-round P2P promotions under
203+
one `kv_request_id` (see "Root causes resolved" above). #49820 now has an
204+
open fix PR (#49877) VALIDATED at 3 levels including our own independent GPU
205+
repro -- works end-to-end, one narrower gap remains (see "PR #49877" above).
206+
#49829 has an open draft fix PR with 2 unresolved correctness findings
207+
(#49850). Until #49877 and #49850 both merge (with #49877's finish()-gap
208+
addressed and #49850's findings addressed), these remain the current
209+
load-readiness blockers -- though the core mechanism is now proven fixable
210+
and the fix proven to work under load, which is a materially stronger
211+
position than "root cause known, no fix."
161212

162213
VERIFIED 2026-07-25 against `main`@70009fb9: `TieringManager.lookup()` still
163214
returns `LookupResult.HIT_PENDING` bare, with no deadline. Upstream added only

0 commit comments

Comments
 (0)