Skip to content

fix(CUDA): Do not throw from ~CudaHandle while the driver is unloading - #4531

Open
inureyes wants to merge 1 commit into
ml-explore:mainfrom
inureyes:fix/cuda-handle-destructor-no-throw
Open

inureyes wants to merge 1 commit into
ml-explore:mainfrom
inureyes:fix/cuda-handle-destructor-no-throw

Conversation

@inureyes

Copy link
Copy Markdown

Problem

~CudaHandle calls reset(), which throws through CHECK_CUDA_ERROR. A throw escaping a destructor calls std::terminate.

The cudaPeekAtLastError() guard above it does not cover driver unloading, which is not a sticky per-context error: the peek returns cudaSuccess, reset() proceeds, Destroy returns cudaErrorCudartUnloading, and the process aborts.

terminate called after throwing an instance of 'std::runtime_error'
  what():  Destroy(handle_) failed: driver shutting down

Seen on Linux / CUDA (GB10, sm_121, CUDA 13. DGX Spark) as an intermittent abort in test binaries after the runner had already reported success, turning green runs into a non-zero exit. Load dependent: roughly one run in ten when another process shares the GPU, rare on an idle machine.

Fix

Release the handle in the destructor without checking the result. reset() is unchanged, so operator= and explicit callers keep their error checking, where throwing is legal. The handle cannot be reclaimed once the runtime is gone, so there is nothing to recover and nothing to report.

Why it is reached that late

#4480 leaked the global CommandEncoder map for this class of problem. Two owners it did not cover can still destroy a CudaHandle after the primary context is released:

  • get_command_encoders() returns a static thread_local map, destroyed at thread exit; each CommandEncoder owns a CudaStream, a CudaGraph and an LRUCache<CudaGraphExec>.
  • Worker::start() detaches its thread, and ~CommandEncoder only signals stop() without waiting, so the detached thread and its CudaStream outlive the encoder by an unsynchronized amount.

This change makes late destruction safe for every owner rather than fatal. Whether those two should also be leaked as in #4480, or joined instead of detached, looks like a separate decision: the detach carries a comment about a Windows join deadlock, and leaking a thread_local costs more than leaking one global.

Testing

Tested on DGX Spark, Linux / CUDA (GB10, sm_121, CUDA 13.0).

  • pre-commit run --all is clean.

  • python3 python/tests/run.py ran 905 tests with one failure, test_linalg.TestLinalg.test_eigh, an eigenvalue tolerance comparison against numpy.

    • That failure is pre-existing on this configuration rather than a regression. No test aborted.
  • No automated regression test accompanies this.

    • Reproducing the failure requires a CudaHandle to be destroyed after the CUDA runtime has begun unloading. That ordering only exists during process teardown, after the test framework itself is gone, so it cannot be triggered deterministically from inside a test. Making it testable would mean injecting the Destroy result, a larger change than the fix it would cover.

It was instead verified by repetition under the condition that provokes it: 40 runs of two test subsets with a sibling process holding the GPU throughout, no abort and no non-zero exit, against a baseline near one in ten.

  • ☑️ I understand it is strictly prohibited to use AI to write PR description
  • AI usage disclosure:

`~CudaHandle` calls `reset()`, which checks the result through `CHECK_CUDA_ERROR` and throws on failure. A throw that escapes a destructor calls `std::terminate`, so this aborts the process rather than reporting anything.

It is reachable at ordinary shutdown. The existing guard tests `cudaPeekAtLastError()`, which catches a sticky per-context error but not the CUDA runtime unloading: once teardown has begun the peek still returns `cudaSuccess`, `reset()` proceeds, `Destroy` returns `cudaErrorCudartUnloading`, and the throw fires. The symptom is a SIGABRT with `Destroy(handle_) failed: driver shutting down` after the program has otherwise finished.

The destructor now releases the handle without checking. `reset()` is unchanged, so `operator=` and explicit callers keep their error checking, where throwing is legal. Nothing is lost by not reporting in the destructor: the handle cannot be reclaimed once the runtime is gone.

Related: ml-explore#4480 leaked the global `CommandEncoder` map for the same class of teardown problem. The `thread_local` map in `get_command_encoders()` (`device.cpp`) is not leaked, and `Worker` detaches its thread while `~CommandEncoder` only signals `stop()` without waiting, so `CudaStream`, `CudaGraph` and `CudaGraphExec` instances can still be destroyed after the primary context is released. This change makes that safe rather than fatal; whether those owners should also be leaked or joined is a separate question.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant