Skip to content

fix+test: combined bug-fixing and test cases - #828

Open
kaiqiy-nv wants to merge 14 commits into
NVIDIA:mainfrom
kaiqiy-nv:kaiqiy/combined-bug-fix-test-cases
Open

kaiqiy-nv wants to merge 14 commits into
NVIDIA:mainfrom
kaiqiy-nv:kaiqiy/combined-bug-fix-test-cases

Conversation

@kaiqiy-nv

Copy link
Copy Markdown
Collaborator

This PR is a combined one.
It tries to quick-fix 2 issues in [B] 6770739 and 1 issue in [B] 6770754.
It also adds lots of more tests to improve the code coverage.

The ONLY product code change is DeviceGraphRingConsumer.cpp, where the 3 issues are located.

In this suggested fix:
We replace throwing CUDA_QEC_ERROR calls in status-returning paths with non-throwing cudaq::qec::error calls, so populate_device_call() can return false and the C factory can return nullptr. We also separate the cudaMalloc and cudaMemset checks, and free d_stats_ before throwing when its initialisation fails.
In this case, pinned function-table memory is released after DEVICE_CALL population failures, the C factory honours its null-on-failure contract, and allocated device memory is released after a cudaMemset failure. Therefore, resource leaks and uncontrolled startup termination can be avoided.

Added test cases for these 3 issues:

  • DeviceGraphRingConsumer.CudaAndDlsymFailures:
    We inject missing and invalid DEVICE_CALL populate entries and a cudaMemset failure, and then check that construction throws and live_count() is zero, so the resource leaks can be exposed.
  • DeviceGraphRingConsumer.SuccessShutdownDispatchedAndCAbi:
    We pass null graph resources to the C factory and then check that it returns nullptr, so an exception escaping instead of the documented null result can be exposed.

Other added test cases are normal ones for code coverage improvement only. No more bugs exposed.

Thanks for looking at this PR.

@kaiqiy-nv
kaiqiy-nv requested a review from bmhowe23 September 14, 2026 05:35
@copy-pr-bot

copy-pr-bot Bot commented Sep 14, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@kaiqiy-nv
kaiqiy-nv force-pushed the kaiqiy/combined-bug-fix-test-cases branch from c42639f to 99f1956 Compare September 15, 2026 01:48
@bmhowe23

Copy link
Copy Markdown
Collaborator

/ok to test 99f1956

@kaiqiy-nv
kaiqiy-nv force-pushed the kaiqiy/combined-bug-fix-test-cases branch from 99f1956 to 0437f77 Compare September 16, 2026 02:29
@kaiqiy-nv

Copy link
Copy Markdown
Collaborator Author

/ok to test 0437f77

g_mode = mode;
g_destroy_graph = 0;
g_destroy_exec = 0;
#ifndef TRT_TEST_UINT8_ONNX_PATH

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The baseline/"success" run_mode here still calls the real decoder::get()/decode(), which goes through actual CUDA graph capture (__real_cudaStreamBeginCapture/__real_cudaGraphInstantiate) rather than the wrapped stubs used by the other failure-injection modes. On a CI runner where the TRT decoder is built but no GPU is present, this call can fail for reasons unrelated to the test's injected-failure scenarios, so HelperModes ends up GPU-availability-dependent rather than deterministically isolating each injected failure path. Could we stub/wrap the baseline case too (or explicitly gate/skip it when no GPU is available) so the test isolates the injected failures cleanly?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.
We now have the explicit-gating option in libs/qec/unittests/decoders/trt_decoder/test_trt_cuda_fail.cpp:123–137.
It checks cudaGetDeviceCount() and skips before launching any helper, so CPU runners never enter the real TRT/CUDA graph path, while GPU runners still exercise each injected wrapper mode.

if (std::strcmp(name, "missing") == 0)
return call_main({"--config=/no/such/qec-cc.yaml"}) == 1 ? 0 : 2;
if (std::strcmp(name, "empty") == 0) {
auto p = qec_cc::write_temp("decoders: []\n");

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

run_helper() writes ~20 temp config files via qec_cc::write_temp() but nothing removes them afterward (no std::filesystem::remove calls in this file) — unlike test_decoding_server_core.cpp/lifecycle.cpp, which do clean up. Same gap exists in test_decoding_server_cqr.cpp around line 105. On long-lived CI containers/runners this leaves orphaned qec-cc-<pid>-<n>.yaml files accumulating over repeated runs. Worth adding cleanup (e.g. an RAII temp-dir/file guard or an explicit teardown) to match the other test files' pattern?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sure.
libs/qec/unittests/qec_cc_test_helpers.h:41–76 now wraps all write_temp() outputs in a process-scoped RAII registry.
CLI helpers and the CQR test register each file, which is removed on normal process exit, including early-return paths.

Signed-off-by: Kaiqi Yan <kaiqiy@nvidia.com>
Signed-off-by: Kaiqi Yan <kaiqiy@nvidia.com>
Signed-off-by: Kaiqi Yan <kaiqiy@nvidia.com>
Signed-off-by: Kaiqi Yan <kaiqiy@nvidia.com>
Signed-off-by: Kaiqi Yan <kaiqiy@nvidia.com>
Signed-off-by: Kaiqi Yan <kaiqiy@nvidia.com>
Signed-off-by: Kaiqi Yan <kaiqiy@nvidia.com>
Signed-off-by: Kaiqi Yan <kaiqiy@nvidia.com>
Signed-off-by: Kaiqi Yan <kaiqiy@nvidia.com>
…thout a GPU, and skip TRT helper without a GPU or ONNX fixture

Signed-off-by: Kaiqi Yan <kaiqiy@nvidia.com>
Signed-off-by: Kaiqi Yan <kaiqiy@nvidia.com>
Signed-off-by: Kaiqi Yan <kaiqiy@nvidia.com>
@kaiqiy-nv
kaiqiy-nv force-pushed the kaiqiy/combined-bug-fix-test-cases branch from 0437f77 to 3c36d60 Compare September 16, 2026 05:40
@kaiqiy-nv

Copy link
Copy Markdown
Collaborator Author

/ok to test 3c36d60

Signed-off-by: Kaiqi Yan <kaiqiy@nvidia.com>
@kaiqiy-nv

Copy link
Copy Markdown
Collaborator Author

/ok to test 72bf7fc

Signed-off-by: Kaiqi Yan <kaiqiy@nvidia.com>
@kaiqiy-nv

Copy link
Copy Markdown
Collaborator Author

/ok to test 3cebbaf

@kaiqiy-nv
kaiqiy-nv requested a review from bmhowe23 September 16, 2026 08:16
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.

2 participants