Skip to content

Two resource leaks on Buffer construction error paths (multicast handle, IPC fds) #17

Description

@yurekami

Two acquire-without-finally sites in Buffer construction leak on error paths. Both are one-line fixes using bindings that already exist, and both are asymmetric with adjacent code in the same file that does guard correctly — which is what makes me think they're oversights rather than intent.

Line numbers are master @ 0f385f0.

1. Multicast handle, moonep/buffer.py:227-241

The handle is acquired, then two Python-level raise sites run before anything can release it:

    if is_root:
        mc_handle, mc_fd = nvl_multicast_create(size_bytes, world_size)
    else:
        mc_handle, mc_fd = 0, None

    fds = _exchange_ipc_fds(mc_fd, [0], local_rank, world_size, group)
    if is_root:
        os.close(mc_fd)

_exchange_ipc_fds can raise — it runs sock.recvmsg under a settimeout(120) (:84) and has an explicit raise RuntimeError("received IPC message without an fd") (:106). If it does, mc_handle is never released, and on the root mc_fd leaks too since os.close(mc_fd) sits after the call. dist.barrier a few lines down is a second raise site inside the same window.

There is a try/finally immediately below, but it covers only root_fd:

    root_fd = fds[0]
    try:
        if not is_root:
            mc_handle = nvl_multicast_import(root_fd)
    finally:
        os.close(root_fd)

The asymmetry: create_nvl_dist_tensor at :161-167 does guard its handle —

    keepalive, local_fd, owned_handle = nvl_dist_alloc(shape=chunk_shape, dtype=dtype)
    try:
        return _map_nvl_dist_tensor(...)
    finally:
        nvl_release_mem_handle(owned_handle)

A multicast object handle is a CUmemGenericAllocationHandle like any other, so the existing nvl_release_mem_handle binding (csrc/bindings.cu:57-59) releases it — no new export needed.

2. IPC fds, moonep/buffer.py:126-128

Same shape, one function up:

    fds = _exchange_ipc_fds(local_fd, list(range(world_size)),
                            local_rank, world_size, group)
    os.close(local_fd)

os.close(local_fd) is a bare next statement, not a finally. And the fds that _exchange_ipc_fds already collected are not closed on its own error path — its finally (:107-114) does only:

    finally:
        sock.close()
        dist.barrier(group=group)
        if local_rank == 0:
            shutil.rmtree(dir_path, ignore_errors=True)

sock.close() does not close SCM_RIGHTS-duplicated descriptors; they're plain ints with no Python owner. Again the contrast is local — all_fds immediately below is guarded:

    all_fds = [fds[r] for r in range(world_size)]
    try:
        full_tensor = nvl_dist_map(...)
    finally:
        for fd in all_fds:
            os.close(fd)

An exported fd also keeps the cuMemCreate allocation alive: cuMemRelease only frees once all mappings are unmapped and all references to the handle including shareable counterparts are released. So the nvl_release_mem_handle(owned_handle) in the caller's finally will not actually free the chunk while a leaked fd is open — for the production caller that chunk is hidden_buf, NvS_padded × H bfloat16 (api.py:352).

The same unguarded pattern appears again at :272-276 in create_nvl_single_owner_tensor.

Severity

Low, and I want to be explicit about why rather than overstate it. All of these paths are already fatal — the failure modes leave the process group desynchronised (the finally at :111 runs a dist.barrier on the failing rank while its peers have moved on), so a single-rank retry isn't a realistic recovery and these can't accumulate across iterations. The leak is process-scoped and the driver reclaims at exit. Most failures in the C++ layer also hit CUCHECKexit(EXIT_FAILURE) before a leak could matter.

What makes it worth fixing anyway is that the fix is trivial and the surrounding code already establishes the pattern, so the diff is small and uncontroversial.

Suggested fix

Wrap each acquisition in try/finally mirroring :161-167, releasing mc_handle via nvl_release_mem_handle and closing any fds already present in the dict before re-raising.

Verification

  • All quotes and line numbers read from master @ 0f385f0 this session, clean working tree.
  • Not executed. No GPU/NVLink here, so I have not built moonep._C or forced either error path. Everything above is from reading the tree — in particular I have not confirmed empirically that a leaked exported fd blocks cuMemRelease on this code path, that part is from the CUDA driver docs.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions