Fix stale test debug callback

[jkwak-work]

Fixes #11720

Motivation

Running slang-test on Windows with debug layers enabled could fail when tests ran in-process without a test server. The minimized reproducer was gfx-unit-test-tool/pointerInBufferRoundtripVulkan.internal, and the broader issue showed later tests such as tests/preprocessor/duplicate-include/a.slang failing after earlier GPU/debug-layer output had already been emitted.

The test harness was passing RHI devices a debug callback bridge whose target CoreDebugCallback belonged to the currently active test invocation. Some RHI objects and test-tool shared-library state can outlive that invocation, so the bridge could retain a pointer to stale per-test callback storage or forward a late message to the wrong test.

Proposed solution

Give each harness invocation its own retained RHI-facing bridge anywhere a device or loaded tool can retain the callback, and bind that bridge to the current CoreDebugCallback only for the active invocation. renderer_test::ScopedCoreDebugCallback sets the inner core callback on entry and clears it on exit, so late debug-layer messages cannot dereference stale callback storage or leak into the next test. Distinct retained bridges also prevent an old RHI emitter from forwarding into a later test's active callback. The bridge serializes binding changes with message forwarding, and CoreDebugCallback serializes buffer access, because RHI debug callbacks can be invoked from backend or driver threads.

Change summary

tools/render-test/slang-support.h:21 adds a mutex-protected, reference-counted CoreToRHIDebugBridge, tools/render-test/slang-support.h:58 adds createRetainedCoreToRHIDebugBridge, tools/render-test/slang-support.h:70 adds ScopedCoreDebugCallback, and tools/render-test/slang-support.h:111 makes CoreDebugCallback buffer access thread-safe. The helper documents that messages arriving while no scoped callback is active are intentionally dropped instead of forwarded to stale per-test callback storage, and its comments record the thread-safety and retained-lifetime contracts.

tools/render-test/render-test-main.cpp:1772 creates a retained bridge for that render-test invocation, binds it around execution, and passes that same bridge to RHI device creation at tools/render-test/render-test-main.cpp:1912.

runUnitTestModule in tools/slang-test/slang-test-main.cpp creates a retained RHI bridge for each in-process unit test at tools/slang-test/slang-test-main.cpp:5688, passes it through UnitTestContext at tools/slang-test/slang-test-main.cpp:5689, and binds it only around that test at tools/slang-test/slang-test-main.cpp:5690.

tools/test-server/test-server-main.cpp:546 mirrors the unit-test server path by creating a retained RHI bridge per RPC and binding it to the per-RPC CoreDebugCallback only while executing that unit test.

tools/gfx-unit-test/scoped-core-debug-callback-test.cpp:29 covers the bridge lifetime contract: a retained bridge is a no-op after the scoped binding exits, a later scoped binding receives only its own messages, sequential scoped bindings do not leak messages between callbacks, and exception unwinding clears the bridge. tools/gfx-unit-test/scoped-core-debug-callback-test.cpp:102 mirrors the production shape with distinct retained bridges and stack callbacks, and tools/gfx-unit-test/scoped-core-debug-callback-test.cpp:116 exercises concurrent debug-message delivery, binding clear, and harness reads.

Concepts and vocabulary

CoreToRHIDebugBridge is the object passed to RHI as an rhi::IDebugCallback; it forwards RHI messages into Slang core's IDebugCallback. RHI may call this object from backend or driver threads, so binding changes and forwarded messages must be synchronized.

CoreDebugCallback is the per-test collector that stores debug-layer error messages so slang-test can report them with the originating test result.

The RHI debug layer can emit messages from driver or validation objects whose lifetime is not identical to a single test function call, so the callback object passed to RHI must not be stack storage for a narrower lifetime.

Reviewer Directives (maintained by agent)

• [LLM github-actions review] Messages delivered to CoreToRHIDebugBridge while no scoped callback is active are intentionally dropped; do not forward them to a previous or next per-test callback. (Fix stale test debug callback #11785 (comment))

Process report

The relevant input shape is valid: RHI code is allowed to retain the callback pointer for the lifetime of devices and validation/debug objects, and the test infrastructure intentionally reuses loaded tools and can keep RHI state alive beyond a single test invocation. The producer should not be changed to assume every debug message is synchronous with one test function. The harness must instead make the callback object lifetime match the retained pointer.

For render-test execution, _innerMain in tools/render-test/render-test-main.cpp creates one retained bridge for the invocation and passes that bridge into RHI device creation. The scoped binder clears the bridge's inner core callback before returning to spawnAndWaitSharedLibrary, while the retained bridge object itself remains alive for any RHI state that still holds the raw callback pointer.

For in-process unit tests, runUnitTestModule in tools/slang-test/slang-test-main.cpp previously created the bridge as ordinary local storage and reused one CoreDebugCallback while iterating tests. A late RHI message after a test completed could therefore be delivered outside that test's reporting scope, and a retained callback pointer could outlive the local bridge. Creating a retained bridge per test gives retained RHI state a stable callback object without sharing that object with later tests. Binding only around the active test means delayed messages from that test's RHI objects hit that test's cleared bridge and are dropped rather than being attributed to a later test.

For test-server unit tests, _executeUnitTest in tools/test-server/test-server-main.cpp has the same callback-retention problem across one RPC. The server-side change applies the same lifetime rule: a retained bridge per RPC for RHI, a scoped per-RPC core callback for reporting.

Reviewer feedback noted that RHI callbacks can arrive on backend or driver threads. The bridge now protects its inner callback pointer with a mutex and holds that mutex while forwarding a message, so ScopedCoreDebugCallback destruction cannot clear the pointer and let the per-test callback die while an in-flight forwarded message is still using it. CoreDebugCallback also protects handleMessage, clear, and getString with a mutex so concurrent RHI messages cannot race on the StringBuilder. coreDebugBridgeHandlesConcurrentMessages exercises this invariant with concurrent writers, scoped binding teardown while writers are active, and a concurrent reader so sanitizer configurations can catch accidental unsynchronized access.

Reviewer feedback asked whether clearing the bridge silently drops late messages. That drop is intentional and belongs at this layer: while no scoped binding is active there is no current test result that can own the message, and forwarding to a destroyed or subsequent per-test CoreDebugCallback is the stale-pointer/cross-test contamination bug this PR fixes. Later feedback pointed out that a single shared retained bridge could still forward an old emitter's delayed message into a later active scope. The fix is therefore one retained bridge per invocation: old emitters call the old bridge, whose scoped binding has been cleared, while the later invocation uses a distinct bridge. The helper now documents that contract, and scopedCoreDebugCallbackClearsBridgeOnExit, scopedCoreDebugCallbackDoesNotLeakAcrossScopes, scopedCoreDebugCallbackClearsBridgeOnException, and scopedCoreDebugCallbackSeparatesRetainedBridgeScopes pin the no-op-after-scope, no-cross-scope-leak, exception-unwind, and retained-bridge separation behavior.

Reviewer feedback clarified that ExecuteResult::debugLayer is not exclusively RHI validation output for ordinary in-process tool execution: StdWriters::setDebugCallback can also route compiler diagnostics into that field. This PR therefore leaves the existing _validateOutput force-failure behavior unchanged outside the unit-test debug-layer paths, so negative compile tests that intentionally produce diagnostics are not failed merely because that diagnostic channel is non-empty.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This PR scopes debug-callback binding, adds mutex synchronization around callback access, updates three test runners to use the scoped helper, and adds unit tests for scope exit, isolation, exception unwinding, retained bridge reuse, and concurrent message handling.

Changes

Scoped debug callback lifetime updates

Layer / File(s)	Summary
Scoped debug callback helper tools/render-test/slang-support.h	CoreToRHIDebugBridge and CoreDebugCallback are updated with mutex-protected callback/message handling, and ScopedCoreDebugCallback is introduced as an RAII wrapper that sets and clears the bridge callback across a scope.
Callback wiring in test runners tools/render-test/render-test-main.cpp, tools/slang-test/slang-test-main.cpp, tools/test-server/test-server-main.cpp	render-test, slang-test, and test-server switch to scoped core debug callback binding, and slang-test makes its RHI debug bridge retained per test run in the non-test-server execution path.
Scoped callback tests tools/gfx-unit-test/scoped-core-debug-callback-test.cpp	Unit tests add scoped-callback coverage for scope exit, sequential scope isolation, exception unwinding, retained bridge reuse, and concurrent message handling.

Suggested reviewers

• bmillsNV
• csyonghe

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and accurately describes the main fix to stale test debug callback handling.
Description check	✅ Passed	The description is directly related and explains the callback lifetime bug and scoped fix.
Linked Issues check	✅ Passed	The PR adds retained bridges, scoped binding, and thread-safe handling across the affected harnesses, matching #11720.
Out of Scope Changes check	✅ Passed	The changes stay focused on the debug callback lifetime fix and related tests, with no clear unrelated additions.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 2c2326a3-efb4-40cc-a7be-e9771d91ad5b

📥 Commits

Reviewing files that changed from the base of the PR and between b1ae635 and ed5ecde.

📒 Files selected for processing (2)

• tools/gfx-unit-test/scoped-core-debug-callback-test.cpp
• tools/render-test/slang-support.h

[coderabbitai]

Actionable comments posted: 1

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 7da67062-a749-4fe4-8dd5-526260677c34

📥 Commits

Reviewing files that changed from the base of the PR and between 31df95c and 0389951.

📒 Files selected for processing (4)

• tools/gfx-unit-test/scoped-core-debug-callback-test.cpp
• tools/render-test/slang-support.h
• tools/slang-test/slang-test-main.cpp
• tools/test-server/test-server-main.cpp

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

tools/slang-test/slang-test-main.cpp (1)

5690-5700: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Close the scoped bridge before taking the final debug-layer snapshot.

Line 5700 reads the buffer while scopedDebugCallback is still bound. A validation-thread message can append after this read but before the scoped object destructs, so the current test can pass despite a debug-layer error; end the scoped binding first, then call getString().

Proposed fix

-            renderer_test::ScopedCoreDebugCallback scopedDebugCallback(
-                *rhiDebugBridge,
-                &coreDebugCallback);
-
             try
             {
-                slang_replayMarker(test.testName.getBuffer());
-                test.testFunc(&unitTestContext);
+                {
+                    renderer_test::ScopedCoreDebugCallback scopedDebugCallback(
+                        *rhiDebugBridge,
+                        &coreDebugCallback);
+                    slang_replayMarker(test.testName.getBuffer());
+                    test.testFunc(&unitTestContext);
+                }
 
                 // Check for VVL errors after test completion
                 String debugMessages = coreDebugCallback.getString();

tools/test-server/test-server-main.cpp (1)

546-584: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Unbind before serializing debugLayer into the RPC result.

Line 584 captures coreDebugCallback while scopedDebugCallback is still active. A late validation message can arrive after the snapshot and be omitted from the RPC response, so slang-test can miss a debug-layer failure.

Proposed fix

-    renderer_test::ScopedCoreDebugCallback scopedDebugCallback(
-        *rhiDebugCallback,
-        &coreDebugCallback);
-
     testModule->setTestReporter(&testReporter);
 
@@
     try
     {
-        testFunc(&unitTestContext);
+        {
+            renderer_test::ScopedCoreDebugCallback scopedDebugCallback(
+                *rhiDebugCallback,
+                &coreDebugCallback);
+            testFunc(&unitTestContext);
+        }
     }
     catch (...)
     {
         testReporter.m_failCount++;

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 4a5e7e9b-80f5-4710-8494-4e5c6e3e2880

📥 Commits

Reviewing files that changed from the base of the PR and between 0389951 and ea591de.

📒 Files selected for processing (5)

• tools/gfx-unit-test/scoped-core-debug-callback-test.cpp
• tools/render-test/render-test-main.cpp
• tools/render-test/slang-support.h
• tools/slang-test/slang-test-main.cpp
• tools/test-server/test-server-main.cpp

[github-actions]

Verdict: ✅ Clean — no significant issues found

Replaces a process-static CoreToRHIDebugBridge with per-invocation retained, ref-counted, mutex-protected bridges plus an RAII ScopedCoreDebugCallback that binds/unbinds the bridge's inner callback only while a test is active. RHI may retain the bridge pointer across invocations; the per-test core callback is bound only for the active scope, so late RHI messages either land on a cleared bridge or a different bridge belonging to the prior invocation.

Changes Overview

Bridge / scoped binder (tools/render-test/slang-support.h)

• CoreToRHIDebugBridge now inherits Slang::RefObject and serializes setCoreCallback / handleMessage on a std::mutex. The forward path holds the mutex across coreCallback->handleMessage(...), so ~ScopedCoreDebugCallback's setCoreCallback(nullptr) cannot return while an in-flight forward is still dereferencing the inner callback.
• New createRetainedCoreToRHIDebugBridge() factory parks each created bridge in an inline-function-local static List<RefPtr<...>>, guaranteeing the bridge survives any RHI-retained raw pointer for the rest of the process. The function-local statics use the leak-on-purpose new idiom to avoid static-destruction-order issues across TUs.
• New ScopedCoreDebugCallback RAII helper binds the bridge's inner callback on construction and clears it on destruction; copy/assign deleted.
• CoreDebugCallback::handleMessage / clear / getString now lock m_mutex so concurrent RHI writes and harness reads do not race on the StringBuilder.

Call sites (tools/render-test/render-test-main.cpp, tools/slang-test/slang-test-main.cpp, tools/test-server/test-server-main.cpp)

• Each harness entry point creates a fresh retained bridge, hands its .Ptr() to RHI device / unit-test-context creation, and wraps execution in a ScopedCoreDebugCallback. In slang-test-main.cpp, the scope is per-test inside the iteration loop and coreDebugCallback.clear() runs immediately before the scope binds, so each test sees only its own messages.

Regression tests (tools/gfx-unit-test/scoped-core-debug-callback-test.cpp, new)

• scopedCoreDebugCallbackClearsBridgeOnExit — messages after scope exit are dropped.
• scopedCoreDebugCallbackDoesNotLeakAcrossScopes — sequential bindings keep each callback's buffer isolated.
• scopedCoreDebugCallbackClearsBridgeOnException — exception unwind still clears the bridge.
• scopedCoreDebugCallbackSeparatesRetainedBridgeScopes — emitting via a retained oldBridge from an earlier scope, while a new scope binds nextBridge, does not contaminate nextCallback (this is the cross-test-contamination scenario the PR fixes).
• coreDebugBridgeHandlesConcurrentMessages — four writer threads × 1024 messages plus a concurrent reader; asserts capturedLength > 0, bounded by kThreadCount * kMessageCount * 2, and (capturedLength % 2) == 0 (each emitError writes "x\n" atomically under the callback mutex). All threads are joined before stack-local bridge/callback destruct.