fix(LOAD-MODELOPT-NVFP4-BORROW): one owner per device resident

.agents/issues/LOAD-MODELOPT-NVFP4-BORROW/ISSUE-LOCAL-01M3QDT6A8JEM8WNJTPR16MXHR.md

@@ -0,0 +1,19 @@
+ID: ISSUE-LOCAL-01M3QDT6A8JEM8WNJTPR16MXHR
+Title: Every Marlin NVFP4 repack leaks its packed and scale device buffers, because Nvfp4Weight publishes one allocation on two owning handles
+Row: LOAD-MODELOPT-NVFP4-BORROW
+State: OPEN
+Kind: bug
+GitHub: -
+Mirror: PENDING
+Availability: FULL
+Created: 2026-09-29
+Updated: 2026-09-29
+Closed: -
+
+## Problem
+
+Nvfp4Weight carries TWO owning handles per buffer: the type-specific d_packed/d_scale members (include/vllm/model_executor/models/qwen3_5_weights.h:707-708) and the generic raw-twin slots packed.d_dev/scale.d_dev (qwen3_5_weights.h:195) that AdoptDeviceBytesAsHost keys on (src/vllm/model_executor/models/qwen3_5_weights.cpp:420-429). ResidentNvfp4 publishes the SAME allocation on both: w.d_packed = shared_ptr(p, Free) followed by w.packed.d_dev = w.d_packed (dense_nvfp4_gemm.h:319-347, twin at src/vllm/model_executor/models/qwen3_5.cpp:1449). Releasing a repacked weight means dropping both handles, but every Marlin repack builder drops only the type-specific pair: dense_nvfp4_gemm.h:448-449, dense_nvfp4_gemm.h:667-670 (both operands of the pair), qwen3_5.cpp:2952-2953, qwen3_5.cpp:3132-3135, qwen3_5.cpp:6890-6891 and laguna.cpp:650-655. The surviving packed.d_dev alias holds the control block, so each repacked weight keeps a full packed+scale device copy alive for the process lifetime; on an MoE model that is one leak per expert per projection. The repack is precisely the step that is supposed to keep peak weight memory flat (dense_nvfp4_gemm.h:395-397: "we do this repack lazily on first forward and then FREE the fp4 originals"), so the leak silently undoes the memory design on exactly the cards that need it: the NVFP4 arms are served on 24 GiB consumer Blackwell (sm_120a), where a second full copy of the fp4 originals does not fit beside the repacked weights. Cause is grounded in the source above, not inferred from a failed allocation.
+
+## Resolution
+
+-

.agents/specs/nvfp4-one-owner-per-resident.md

@@ -0,0 +1,115 @@
+# `Nvfp4Weight` publishes one device allocation on two owning handles — ISSUE-LOCAL-01M3QDT6A8JEM8WNJTPR16MXHR
+
+`ResidentNvfp4` hands the same device block to `Nvfp4Weight::d_packed` and to
+`OwnedTensor::d_dev`. Every Marlin repack builder released only the first, so the
+second kept a full packed+scale device copy alive for the process lifetime — once
+per repacked weight, and once per expert per projection on an MoE model. The
+repack is the step that is supposed to keep peak weight memory flat, so the
+surviving alias silently undoes the memory design on exactly the cards that need
+it: the NVFP4 arms serve on 24 GiB consumer Blackwell (`sm_120a`), where a second
+full copy of the fp4 originals does not fit beside the repacked weights.
+
+Issue: [ISSUE-LOCAL-01M3QDT6A8JEM8WNJTPR16MXHR](../issues/LOAD-MODELOPT-NVFP4-BORROW/ISSUE-LOCAL-01M3QDT6A8JEM8WNJTPR16MXHR.md).
+Owning row: `LOAD-MODELOPT-NVFP4-BORROW` ([engine-matrix.md](../engine-matrix.md)),
+the row that owns `ResidentNvfp4`'s upload and adoption path and the spec
+[`load-modelopt-nvfp4-borrow.md`](load-modelopt-nvfp4-borrow.md).
+
+## The defect, grounded
+
+| Where (line anchors at this branch's base, `b45a94273`) | What |
+|---|---|
+| `include/vllm/model_executor/models/qwen3_5_weights.h:707-708` | `Nvfp4Weight::d_packed`/`d_scale`, documented as "the shared_ptr deleter frees through the vt Backend". |
+| `.../qwen3_5_weights.h:195` | `OwnedTensor::d_dev`, the generic raw-twin slot `AdoptDeviceBytesAsHost` keys on (`src/vllm/model_executor/models/qwen3_5_weights.cpp:420-429`). |
+| `.../dense_nvfp4_gemm.h:319-347` | `ResidentNvfp4` sets `w.d_packed = shared_ptr(p, Free)` and then `w.packed.d_dev = w.d_packed`: **two owning handles, one control block.** |
+| `src/vllm/model_executor/models/qwen3_5.cpp:1449-1476` | The private twin of the same function, the same two-handle shape. |
+| `.../dense_nvfp4_gemm.h:448-449`, `:667-670` | `BuildMarlinDenseResident` and `BuildMarlinDensePairResident` release the type-specific pair only. |
+| `src/vllm/model_executor/models/qwen3_5.cpp:2952-2953`, `:3132-3135`, `:6890-6891` | The 27B dense and MoE repack builders, the same release shape. |
+| `src/vllm/model_executor/models/laguna.cpp:650-655` | Six resets per expert, the same shape. |
+
+The alias is not an accident of one builder: it is the design (`packed.d_dev`
+is what `AdoptDeviceBytesAsHost` can act on), and the fix is to make it the ONLY
+owner rather than to teach six call sites to drop two handles.
+
+## The design call, and what it costs
+
+Alternatives rejected:
+
+1. **Keep both handles and reset both at every caller.** Correct at each site and
+   wrong for the codebase: six release sites, one of which (a new builder) will
+   forget. The bug is the duplicated ownership, not the six callers.
+2. **Make `d_dev` observe `d_packed` without owning it** (a raw pointer or a
+   `weak_ptr`). `AdoptDeviceBytesAsHost` needs the block alive while the adopted
+   host view exists, and the residency machinery keys on `d_dev` everywhere else;
+   splitting the lifetime rules between two slots reintroduces the same class of
+   mistake.
+3. **Have `ReleaseResident` free through the deleter directly.** It cannot: the
+   adopted `bytes` view holds a copy of the shared_ptr on a host-addressable
+   device, so the block outlives the weight's handle by construction. The view
+   must be dropped first, which is what `ReleaseResident` does.
+
+## Design
+
+One owner per allocation: `packed.d_dev` / `scale.d_dev`. The type-specific
+`d_packed`/`d_scale` members are deleted, so a reader that still expects them
+fails to **compile** — the migration cannot be half-done.
+
+Two additions carry the release:
+
+- `OwnedTensor::HostViewIsDeviceTwin()` — true when `d_dev` is set, `bytes` is an
+  adopted (borrowed, `mmap_src == nullptr`) view, and it points at that same
+  block. This is the predicate for "the host view is an alias of this tensor's
+  own device allocation", i.e. it holds the block alive.
+- `Nvfp4Weight::ReleaseResident()` — drops an adopted twin view first, then
+  resets `d_dev` on both tensors. Called at all six release sites.
+
+`ResidentNvfp4` (both copies) uploads into `packed.d_dev`/`scale.d_dev`
+directly. The old upload guard `!w.d_packed` becomes `!w.packed.d_dev`; that is
+equivalent because the upload is the only writer of either slot for an
+`Nvfp4Weight`'s packed/scale tensors (`grep -rn 'd_dev = ' src include`).
+
+**Behavior is unchanged: same bytes, same adoption point, same release points.**
+The only new effect is that the adopted alias view is dropped before the block is
+freed, which is required for the free to happen at all.
+
+## What this does NOT claim
+
+No end-to-end GiB number is measured here. The unit gate proves that a release
+through the weight's own resident state frees both buffers; it does not measure
+the process RSS of a real repacking serve on a real checkpoint. That measurement
+needs a model and a GPU and is the natural follow-up, not a precondition for the
+correctness fix.
+
+## Tests
+
+`tests/vllm/test_load_direct_upload.cpp`, which already owns the `ResidentNvfp4`
+accounting/adoption cases over the `FakeBackend` + `ObservableMapping` harness:
+
+- New: `fp4 resident: releasing the resident frees both device buffers`. Uploads
+  a borrowed fp4 weight on a non-host-addressable fake device (no adoption, so
+  the alias is the only extra reference), then performs the release and asserts
+  `b.frees == 2`.
+  - **RED pre-fix**, captured:
+    `CHECK( b.frees == 2 )` → `values: CHECK( 0 == 2 )`, because the release
+    reset only the type-specific pair.
+  - **GREEN post-fix**: the release is `w.ReleaseResident()`.
+- The three existing cases are updated to the single-owner shape
+  (`w.packed.d_dev` / `w.scale.d_dev` in place of `w.d_packed` / `w.d_scale`),
+  and their final `CHECK(b.frees == 2)` (the weight out of scope) is unchanged.
+
+What the gate does not prove: that a real CUDA driver frees at that instant. The
+`FakeBackend` counts `Free` calls; it cannot observe the driver.
+
+## Gates
+
+- `cmake --build build --target test_load_direct_upload && ./build/tests/test_load_direct_upload`
+  — RED before, GREEN after, all 17 cases.
+- The full `ctest --test-dir build` on the affected tree.
+- `scripts/agent-preflight.sh` (the record gates for the changed files).
+- The build itself is the completeness check for the deleted members.
+
+## Stop conditions
+
+- A caller of `d_packed`/`d_scale` outside the sites listed above: stop and
+  re-scope, because the deletion is no longer complete.
+- A regression in the adoption cases (host-addressable device): stop, because
+  the release ordering is wrong.

include/vllm/model_executor/models/dense_nvfp4_gemm.h

@@ -317,38 +317,35 @@ struct Nvfp4Dev {
 };
 
 inline Nvfp4Dev ResidentNvfp4(Dev d, const Nvfp4Weight& w) {
-  if (!w.d_packed) {
+  if (!w.packed.d_dev) {
     const size_t pb = w.packed.bytes.size();
     void* p = d.b.Alloc(pb);
     // ENG-LOAD-DIRECT-UPLOAD (issue #150). `LoadCtNvfp4W4A16`/`LoadCtMxfp4W4A16`
     // /`LoadCtNvfp4Raw` BORROW `packed` and `scale` from the safetensors mmap,
     // so this is the one host->device move of those bytes and it must be
     // accounted and followed by the same post-upload residency step every other
-    // qualifying weight gets. Publishing the allocation on the OwnedTensor is
-    // what lets `AdoptDeviceBytesAsHost` run at all (it keys on `d_dev`); the
-    // two handles share one control block, so the buffer is still freed exactly
-    // once, through the vt Backend.
+    // qualifying weight gets. Publishing on `d_dev` is what lets
+    // `AdoptDeviceBytesAsHost` run (it keys on that slot), and it is the only
+    // owner (Nvfp4Weight::ReleaseResident).
     vllm::load_stats::AddDeviceUpload(pb);
     d.b.Copy(d.q, p, w.packed.bytes.data(), pb);
     Backend* bk = &d.b;
-    w.d_packed = std::shared_ptr<void>(p, [bk](void* q) { bk->Free(q); });
-    w.packed.d_dev = w.d_packed;
+    w.packed.d_dev = std::shared_ptr<void>(p, [bk](void* q) { bk->Free(q); });
     AdoptDeviceBytesAsHost(d.b, w.packed);
   }
-  if (!w.d_scale) {
+  if (!w.scale.d_dev) {
     const size_t sb = w.scale.bytes.size();
     void* p = d.b.Alloc(sb);
     vllm::load_stats::AddDeviceUpload(sb);
     d.b.Copy(d.q, p, w.scale.bytes.data(), sb);
     Backend* bk = &d.b;
-    w.d_scale = std::shared_ptr<void>(p, [bk](void* q) { bk->Free(q); });
-    w.scale.d_dev = w.d_scale;
+    w.scale.d_dev = std::shared_ptr<void>(p, [bk](void* q) { bk->Free(q); });
     AdoptDeviceBytesAsHost(d.b, w.scale);
   }
   Nvfp4Dev r;
-  r.packed = MakeTensor(w.d_packed.get(), DType::kI8, d.q.device, {w.n, w.k / 2});
+  r.packed = MakeTensor(w.packed.d_dev.get(), DType::kI8, d.q.device, {w.n, w.k / 2});
   // Scale grid is [N, K/group_size]: K/16 for NVFP4, K/32 for MXFP4.
-  r.scale = MakeTensor(w.d_scale.get(), DType::kI8, d.q.device, {w.n, w.k / w.group_size});
+  r.scale = MakeTensor(w.scale.d_dev.get(), DType::kI8, d.q.device, {w.n, w.k / w.group_size});
   return r;
 }
 
@@ -445,8 +442,7 @@ inline void BuildMarlinDenseResident(Dev d, const Nvfp4Weight& w,
     d.b.Copy(d.q, mr.g, &g, sizeof(float));
   }
   d.b.Synchronize(d.q);  // repack done -> safe to free the fp4 originals
-  w.d_packed.reset();
-  w.d_scale.reset();
+  w.ReleaseResident();
   mr.ready = true;
 }
 
@@ -664,10 +660,8 @@ inline void BuildMarlinDensePairResident(Dev d, const Nvfp4Weight& gw,
   d.b.Synchronize(d.q);  // repack done -> safe to free staging + fp4 originals
   d.b.Free(tmp_w);
   d.b.Free(tmp_s);
-  gw.d_packed.reset();
-  gw.d_scale.reset();
-  uw.d_packed.reset();
-  uw.d_scale.reset();
+  gw.ReleaseResident();
+  uw.ReleaseResident();
   mr.ready = true;
 }
 

include/vllm/model_executor/models/qwen3_5_weights.h

@@ -179,13 +179,21 @@ struct OwnedTensor {
   // `residency_policy().release_host_weights_after_upload`
   // (platforms/interface.h; BACKEND-PLATFORM item 2). Logically const: the
   // tensor's VALUE is unchanged, only the now-dead host mirror is freed. This
-  // mirrors the existing mutable lazy-device-upload residency design (d_dev/
-  // d_packed above are populated on a const weight). swap-with-empty (not
+  // mirrors the existing mutable lazy-device-upload residency design (d_dev
+  // above is populated on a const weight). swap-with-empty (not
   // clear()) guarantees the std::vector capacity is actually deallocated. After
   // release View()/bytes must not be read; shape/dtype metadata is retained and
   // Empty() remains false so device-resident dispatch continues to see it.
   void ReleaseHost() const;
 
+  // True when the host view is an adopted alias of this tensor's own device
+  // allocation (host-addressable devices: AdoptDeviceBytesAsHost re-points
+  // `bytes` at it and the block becomes the view's keep-alive).
+  bool HostViewIsDeviceTwin() const {
+    return d_dev != nullptr && bytes.borrowed() && mmap_src == nullptr &&
+           bytes.data() == static_cast<const uint8_t*>(d_dev.get());
+  }
+
   // Lazily-populated device-resident copies (CUDA forward only; null on host or
   // before first use). Uploaded ONCE and reused across every forward step so the
   // model's bf16/f32 weights (embed table, norms, attention/GDN projections,
@@ -677,6 +685,16 @@ struct Nvfp4Weight {
   int64_t k = 0;        // in_features (K % 16 == 0)
   bool Empty() const { return packed.Empty(); }
 
+  // Drop this weight's device resident. `packed.d_dev`/`scale.d_dev` are the
+  // only owners, so the resets free; an adopted host view (see
+  // HostViewIsDeviceTwin) must go first because it holds the same block alive.
+  void ReleaseResident() const {
+    if (packed.HostViewIsDeviceTwin()) packed.ReleaseHost();
+    if (scale.HostViewIsDeviceTwin()) scale.ReleaseHost();
+    packed.d_dev.reset();
+    scale.d_dev.reset();
+  }
+
   // Block-scale FORMAT. Default = NVFP4: group_size 16, fp8-e4m3 `scale`, a
   // per-tensor `scale2` global. is_mxfp4 selects compressed-tensors MXFP4
   // (`mxfp4-pack-quantized`): group_size 32, E8M0 (UE8M0) `scale` [N, K/32], NO
@@ -702,13 +720,11 @@ struct Nvfp4Weight {
   // True when the activation-quant globals were loaded (27B true-W4A4 path).
   bool IsTrueW4A4() const { return alpha > 0.0F; }
 
-  // Lazily-populated device-resident copies (CUDA forward only; null on host or
-  // before first use). The shared_ptr deleter frees through the vt Backend.
-  mutable std::shared_ptr<void> d_packed;
-  mutable std::shared_ptr<void> d_scale;
+  // Device residency lives on `packed.d_dev`/`scale.d_dev` (the generic raw-twin
+  // slot): one owner per allocation, no second Nvfp4Weight handle.
   // Lazily-populated SWIZZLED weight block scale for the cutlass sm120a fp4 GEMM
   // path (VT_NVFP4_CUTLASS): [round_up(n,128), round_up(k/16,4)] in the cutlass
-  // atom layout, computed once from d_scale via vt::SwizzleBlockscale.
+  // `scale.d_dev` (the generic raw-twin slot) via vt::SwizzleBlockscale.
   mutable std::shared_ptr<void> d_scale_sw;
   // vLLM/FlashInfer-compatible model-owned f32 alpha for the true-W4A4 CUTLASS
   // path. Uploaded once from the persistent `alpha` member; the diagnostic host