Repository navigation
fix(CUDA): set weights_owned before the host-to-device copy so failed uploads free their buffer - #1108
Closed
monotophic wants to merge 3 commits into
Closed
fix(CUDA): set weights_owned before the host-to-device copy so failed uploads free their buffer#1108monotophic wants to merge 3 commits into
monotophic wants to merge 3 commits into
Conversation
…ee their buffer coli_cuda_tensor_upload marked ownership only after the weight memcpy succeeded, so a malloc-success/memcpy-failure upload freed the tensor while weights_owned was still 0 and coli_cuda_tensor_free's ownership gate skipped the cudaFree: every failed upload leaked weight_bytes of device memory, invisible to the ledger (tracked=0). Ownership is a fact of the allocation, not the copy — set it between the two. tests/test_weights_owned_cuda.cu pins the contract by including the backend directly and renaming its cudaMemcpy call sites to a hook that fails H2D copies of the target byte count: 8 injected create-fail cycles must leave cudaMemGetInfo free memory unchanged (previously a monotonic ~64 MiB/cycle loss), the success-path control must round-trip to zero delta, and the ledger must never see a failed upload. Wired into cuda-test and gpu-compile alongside the other direct-include tests.
…h's mapped surface The hook named cudaMemcpyKind and cudaErrorInvalidValue, which the compat header does not alias (the backend never uses them), so the cuda-test recipe under HIP=1 failed to compile. Compat's precedent is to map only what the backend needs; alias the kind type and the injected error value locally per vendor instead of widening the product header for a test-only seam. CUDA behavior unchanged.
… the HIP build backend_gpu_compat.h maps only the CUDA runtime names backend_cuda.cu itself uses; the injection hook's kind type and injected error value are not among them, so hipcc -- which compiles this file in the HIP syntax-check lane via gpu-compile -- finds them undefined. Alias the two locally under the HIP arm and keep the shared hook body in the cuda spelling, the arrangement the repo's other direct-include tests use for their off-surface names. The hook's forward declaration also no longer leans on hipcc pre-including the runtime header: the HIP arm now includes <hip/hip_runtime.h> explicitly, the same header backend_gpu_compat.h pulls, unreachable under nvcc. CUDA behavior unchanged.
Owner
JustVugg
added a commit
that referenced
this pull request
Aug 19, 2026
fix(CUDA): weights_owned before the H2D copy — rebase of #1108 by @monotophic
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Authored by Fable 5 in Claude Code, analysis in partnership with @monotophic
In the CUDA weight-upload path, ownership (
weights_owned=1) is recordedonly after the H2D
cudaMemcpysucceeds. If the malloc succeeds and thecopy fails, the error path's
coli_cuda_tensor_freesees an un-ownedpointer and skips the
cudaFree— the allocation leaks for real, and thebyte ledger never sees it (the tensor was still untracked). Ownership is
a fact of the allocation, not the copy: this sets it between the malloc
and the memcpy. One line; a device-side injection test locks it.
Behavioral contract
leave device free memory unchanged.
Capstone matrix
make cuda-testgreen on fix head; CPUmake check+ METAL suites greenFuller matrix and review record
Two-reviewer roster + one fix round. The fix round was test-infra only: the injection hook initially used two CUDA symbols outside backend_gpu_compat.h's mapped surface (would break the HIP compile lane); rewritten to the mapped surface, product diff untouched. HIP compile is inspection-verified only (no AMD hardware here) — CI's hip-syntax lane arbitrates, same as prior PRs. Success-path control: 8 create/free cycles, zero free-memory delta, ledger restored, both heads. Pre-existing adjacent observations recorded out of scope: a ledger-counter race under parallel pin_load; ANS async-failure free ordering.Durable vs current-state: the ordering fix is durable; leak-rate
figures are current-state (GB10/sm_121, ~64 MB test tensors, 2026-08-18,
base ad79236).