debug_traceCallMany: extend timeout CTS lifetime to enumerator disposal

[LukaszRozmej]

Replaces #11425

Changes

• Bind the timeout CancellationTokenSource lifetime to enumerator disposal in DebugRpcModule.TraceCallMany's simple path. The previous using scope disposed the CTS before the lazy bundle pipeline was enumerated, leaving the cancellation token unusable to downstream consumers (e.g. WaitHandle access throws ObjectDisposedException).
• Mirrors the iterator-owns-CTS pattern already used by EthRpcModule.GetLogs. Streaming is preserved — no per-bundle or per-trace buffering is introduced.
• Add two regression tests:
  • Debug_traceCallMany_does_not_dispose_timeout_cts_before_consumer_enumerates fails on the unfixed code with ObjectDisposedException and passes with the fix.
  • Debug_traceCallMany_does_not_eagerly_materialize_bundles guards against a regression to eager materialization.

Replaces #11425, which fixed the same bug by materializing every bundle/trace into nested List<>/T[] and giving up the zero-allocation streaming pipeline that was a deliberate design choice (see #11425 (comment)).

Types of changes

What types of changes does your code introduce?

• Bugfix (a non-breaking change that fixes an issue)
• New feature (a non-breaking change that adds functionality)
• Breaking change (a change that causes existing functionality not to work as expected)
• Optimization
• Refactoring
• Documentation update
• Build-related changes
• Other: Description

Testing

Requires testing

• Yes
• No

If yes, did you write tests?

• Yes
• No

Notes on testing

• All 22 DebugModuleTests pass with the fix.
• Verified the new disposed-CTS regression test fails on master without the production fix (ObjectDisposedException from WaitHandle) and passes with it.

Documentation

Requires documentation update

• Yes
• No

Requires explanation in Release Notes

• Yes
• No

[claude]

Claude finished @LukaszRozmej's task in 3m 49s —— View job

---

Review: debug_traceCallMany — extend timeout CTS lifetime to enumerator disposal

• Gather context: load rules and read changed files
• Review production code changes (DebugRpcModule.cs)
• Review test changes (DebugModuleTests.cs)
• Post findings

---

Summary

The bug diagnosis is correct and the fix is sound. The previous using scope disposed the CancellationTokenSource synchronously when TraceCallMany returned, but the token was still live inside the lazy IEnumerable pipeline handed to the JSON-RPC serializer. The StreamBundleTraces iterator pattern correctly ties CTS lifetime to enumerator disposal, exactly mirroring EthRpcModule.GetLogs (line 759).

Critical: 0 · High: 0 · Medium: 0 · Low: 2

---

Production code (DebugRpcModule.cs)

Correctness — ✅ Fix is correct

StreamBundleTraces is a C# iterator method; the using (cancellationTokenSource) block inside it is compiled to a finally clause that runs on all exit paths: normal exhaustion, enumerator disposal (including foreach break/early return by the JSON-RPC serializer), and exceptions propagating out of the generator body. The CTS is guaranteed to outlive the outer enumerable and will be disposed promptly once the caller finishes serializing.

Exception-safety of TraceCallMany — Low (see inline comment at line 468)

The CTS is now heap-allocated without a guard. Everything between BuildTimeoutCancellationTokenSource() and the return is low-risk in practice (a lazy GetBundleTraces call + a conditional logger invocation), but an unexpected exception would leave the CTS undisposed. The EthRpcModule.eth_getFilterLogs reference pattern uses explicit timeout.Dispose() calls in its catch blocks. Not a blocker, but worth tidying for consistency.

Misleading log message — Low (see inline comment at line 476, pre-existing)

The "completed" trace message fires before any tracing work has occurred; it actually means the lazy pipeline was wired, not that it executed.

TraceCallManyWithOverrides is unaffected — ✅

Still correctly uses using CancellationTokenSource timeout = ... because SimulateTxExecutor.Execute is eager and materialises results before the method returns.

---

Tests (DebugModuleTests.cs)

Debug_traceCallMany_does_not_dispose_timeout_cts_before_consumer_enumerates — ✅

Sound regression test. Accessing cancellationToken.WaitHandle on a token whose source has been disposed throws ObjectDisposedException. The test performs that access inside the inner iterator (YieldOne) during serialization-time enumeration (traces.Count()). Pre-fix this reliably explodes; post-fix the CTS is alive. The innerCounts.Should().BeEquivalentTo([1]) assertion also validates end-to-end correctness.

Debug_traceCallMany_does_not_eagerly_materialize_bundles — ✅

YieldOneThenThrow produces a valid first element then throws; if debug_traceCallMany ever eagerly enumerates it (e.g. via .ToList()), the exception surfaces immediately and the test fails. The assertion that result.Data != null and no exception is thrown is sufficient to guard against a regression to eager materialisation.

BuildTraceCallManyModule / CreateDebugRpcModule changes — ✅

The optional-parameter extension to CreateDebugRpcModule is clean and doesn't break any existing tests. The shared BuildTraceCallManyModule helper correctly wires blockFinder.Head, FindHeader, and HasStateForBlock, which are the three lookups debug_traceCallMany performs before dispatching.

---

Verdict

Correct fix, well-motivated, well-tested. The two Low findings (CTS exception-path safety, pre-existing log wording) are editorial and don't block merge.

[LukaszRozmej]

@claude re-review

[claude]

Claude finished @LukaszRozmej's task in 2m 6s —— View job

---

Re-review: debug_traceCallMany — extend timeout CTS lifetime to enumerator disposal

• Load rules files
• Read current state of changed files
• Compare with previous review findings
• Post updated review

---

Summary

Two commits landed since the initial review:

• c99b048 Guard CTS against exceptions before iterator handoff — addresses the Low finding from the previous review
• c2ab959 Simplify debug_traceCallMany regression test — consolidates the two separate regression tests into one

Critical: 0 · High: 0 · Medium: 0 · Low: 0 (all prior findings resolved or acknowledged)

---

Previous Low #1 — Unguarded exception path ✅ Fixed

The try/catch { timeout.Dispose(); throw; } guard is now in place. The exception-path leak is closed. This matches the EthRpcModule pattern.

CancellationTokenSource timeout = BuildTimeoutCancellationTokenSource();
try
{
    ...
    return ResultWrapper<...>.Success(StreamBundleTraces(bundleTraces, timeout));
}
catch
{
    timeout.Dispose();
    throw;
}

On the happy path, ownership transfers to StreamBundleTraces which disposes via its using (cancellationTokenSource) generator-finally. On any exception between creation and handoff, the catch disposes. No path leaks the CTS.

Previous Low #2 — Misleading log message

Still present (pre-existing; acknowledged as out of scope in original review). No action needed.

---

Test consolidation ✅

Debug_traceCallMany_streams_under_live_cancellation_token now covers both concerns in one test:

1. CTS lifetime — YieldTrace accesses token.WaitHandle, which throws ObjectDisposedException on a disposed CTS. If TraceCallMany disposed the CTS before returning, this would fail immediately during outer.Current.Count().

2. Deferred evaluation — StreamBundles throws InvalidOperationException on the second bundle. If TraceCallMany (or StreamBundleTraces) ever called .ToList() or otherwise eagerly evaluated the outer sequence, the exception would surface before the test even reaches the using IEnumerator<...> block.

The outer enumerator's using ensures StreamBundleTraces's generator-finally (cancellationTokenSource.Dispose()) runs on test teardown, so there's no resource leak in the test itself. Clean and correct.

---

Verdict

All prior findings resolved. Fix is correct, well-guarded, and well-tested. Ready to merge.