Skip to content

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

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

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

Conversation

@kaiqiy-nv

@kaiqiy-nv kaiqiy-nv commented Sep 14, 2026 •

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 LOGIC change is DeviceGraphRingConsumer.cpp, where the 3 issues are located.

The only other product side code change (libs/qec/lib/realtime/decoding-server-cqr/CMakeLists.txt) is to register the existing cudaq-qec-decoding-server-device-graph-linkcheck executable with ctest; it does not change the library.

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

@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

@kaiqiy-nv

Copy link
Copy Markdown
Collaborator Author

/ok to test 72bf7fc

@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
@kaiqiy-nv
kaiqiy-nv force-pushed the kaiqiy/combined-bug-fix-test-cases branch from 3cebbaf to cdee142 Compare September 23, 2026 02:15
@kaiqiy-nv

Copy link
Copy Markdown
Collaborator Author

/ok to test cdee142

@kaiqiy-nv
kaiqiy-nv force-pushed the kaiqiy/combined-bug-fix-test-cases branch from cdee142 to d3d711e Compare September 28, 2026 10:36
@kaiqiy-nv

Copy link
Copy Markdown
Collaborator Author

/ok to test d3d711e

@kaiqiy-nv
kaiqiy-nv force-pushed the kaiqiy/combined-bug-fix-test-cases branch from d3d711e to 4dff40e Compare September 30, 2026 08:39
@kaiqiy-nv

Copy link
Copy Markdown
Collaborator Author

/ok to test 4dff40e

@kaiqiy-nv

Copy link
Copy Markdown
Collaborator Author

Hi @bmhowe23 , I think this PR is ready for review again. Thanks.

@kaiqiy-nv
kaiqiy-nv force-pushed the kaiqiy/combined-bug-fix-test-cases branch from 4dff40e to 6595335 Compare October 13, 2026 06:08
@kaiqiy-nv

Copy link
Copy Markdown
Collaborator Author

/ok to test 6595335

@kaiqiy-nv

Copy link
Copy Markdown
Collaborator Author

/ok to test 9eeac99

Comment thread libs/qec/unittests/CMakeLists.txt Resolved
Comment thread libs/qec/unittests/test_cudaq_stubs.cpp Resolved

@bmhowe23 bmhowe23 left a comment

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.

Several of the new tests run code paths without asserting the behavior they're named for (examples inline). Since these count toward coverage but wouldn't catch regressions, could you either add assertions that can actually fail, or drop them? Fewer, sharper tests would make this much easier to review and maintain.

Comment thread libs/qec/unittests/test_decoders.cpp Resolved
@kaiqiy-nv
kaiqiy-nv force-pushed the kaiqiy/combined-bug-fix-test-cases branch from 9eeac99 to 6f7219d Compare October 14, 2026 08:33
@kaiqiy-nv

Copy link
Copy Markdown
Collaborator Author

We also strengthened DecoderStats.LatencyHistogramClampsLowAndHigh, DecoderStats.SubmitDecodeResetTiming, HopStatsHelpers.ScenarioProcesses, TrtCudaFail.HelperModes, DecodingServerCqr.DispatchRpcFailureModesAndShutdown, DeviceGraphRingConsumer.SuccessShutdownDispatchedAndCAbi, and DecoderDemChunksYAMLTest.SeamSpecsMappingRoundTripsWithoutExpand so a broken path cannot pass on a status code alone.

@kaiqiy-nv

Copy link
Copy Markdown
Collaborator Author

/ok to test 6f7219d

@kaiqiy-nv

Copy link
Copy Markdown
Collaborator Author

Hi @bmhowe23 , thanks for the comments, and the code is ready for review again.

@kaiqiy-nv
kaiqiy-nv requested a review from bmhowe23 October 14, 2026 09:22
@kaiqiy-nv
kaiqiy-nv requested a review from bmhowe23 November 5, 2026 11:18
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>
Signed-off-by: Kaiqi Yan <kaiqiy@nvidia.com>
Signed-off-by: Kaiqi Yan <kaiqiy@nvidia.com>
…, and dataplane wraps

Signed-off-by: Kaiqi Yan <kaiqiy@nvidia.com>
Signed-off-by: Kaiqi Yan <kaiqiy@nvidia.com>
…ild when a CUDA-Q stub is missing

Signed-off-by: Kaiqi Yan <kaiqiy@nvidia.com>
…ild when a CUDA-Q stub is missing

Signed-off-by: Kaiqi Yan <kaiqiy@nvidia.com>
…rengthen those tests

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>
@kaiqiy-nv
kaiqiy-nv force-pushed the kaiqiy/combined-bug-fix-test-cases branch from d7baad1 to 69c8dff Compare November 5, 2026 11:29
@kaiqiy-nv

Copy link
Copy Markdown
Collaborator Author

/ok to test 69c8dff

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

Copy link
Copy Markdown
Collaborator Author

/ok to test d68473f

@melody-ren

melody-ren commented Nov 6, 2026 •

Copy link
Copy Markdown
Collaborator

Thanks @kaiqiy-nv for the PR.

For v0.9.0, I would prefer splitting this into two PRs:

  1. The three DeviceGraphRingConsumer.cpp fixes, their regression tests, and only the supporting changes needed to build and run those tests. (We can aim to have this focused PR included in v0.9.0)
    Namely, these files:
libs/qec/lib/realtime/decoding-server-cqr/DeviceGraphRingConsumer.cpp
libs/qec/unittests/test_device_graph.cpp
libs/qec/unittests/CMakeLists.txt
  1. The extended test coverage for other existing functionality. (Defer this for a later release)

This would let us include the fixes in RC3 while reviewing the broader coverage independently.

One additional point: test_decoding_server_cqr is enabled whenever the CQR library exists, but it unconditionally includes the DeviceCallService header and calls cudaqGetDeviceCallServicePluginInfo. The library can be built without that optional shim, so the new test can break otherwise supported configurations. Please gate the service-specific tests on whether the shim is built, or separate them from the base CQR tests.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants