MoonshotAI / MoonshotAI/MoonEP

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

Open Beginner friendly
#17 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
1.1k
Forks
134
PR merge metrics
No merged PRs in 30d

Description

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.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in moonep/buffer.py at the unguarded construction paths around lines 126-128, 227-241, and 272-276, then compare them with the guarded create_nvl_dist_tensor path at lines 161-167. Review the existing nvl_release_mem_handle binding in csrc/bindings.cu and the _exchange_ipc_fds cleanup. Done means error paths release acquired handles and close any collected file descriptors before re-raising.

Written by the indexing model from the issue text.

Assessment

Tech stack
python
Domain
distributed-systems
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.