fix(network): ignore background task cancellation during session shut…

[mk0walsk]

Changes

• Only treat OperationCanceledException as expected when the background task token is canceled during session shutdown or disconnect
• Preserve DisconnectReason.BackgroundTaskFailure for unexpected cancellation paths instead of swallowing all canceled background tasks
• Add regression tests covering both closing and non-closing session behavior

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

• dotnet test --project src/Nethermind/Nethermind.Network.Test/Nethermind.Network.Test.csproj -c Release -- --filter FullyQualifiedName~ProtocolHandlerBaseTests

Documentation

Requires documentation update

• Yes
• No

Requires explanation in Release Notes

• Yes
• No

[batrr]

@claude review

[claude]

Claude finished @batrr's task in 3m 46s —— View job

---

PR Review

• Read rule files (robustness.md, coding-style.md, test-infrastructure.md)
• Read changed files
• Analyze diff
• Post review findings

---

Summary

The fix is logically correct. The original code caught all exceptions (including OperationCanceledException) in the generic catch (Exception e) block, causing spurious BackgroundTaskFailure disconnects during normal session shutdown. The change adds a targeted when filter that silences cancellation only when both the scheduler token is cancelled and the session is already closing — a sound condition for this path.

No critical or high issues. Two findings below.

---

Findings

Medium — Test duplication violates [TestCase] rule

Does_not_disconnect_for_operation_canceled_when_session_is_closing and Disconnects_for_operation_canceled_when_session_is_not_closing differ only in a single boolean (IsClosing) and the expected assertion. The test-infrastructure rules explicitly require [TestCase] parameterization instead of duplicating methods for this pattern.

Suggested refactor (ProtocolHandlerBaseTests.cs):

[TestCase(true)]
[TestCase(false)]
public void Operation_canceled_behavior_depends_on_session_closing(bool sessionIsClosing)
{
    ISession session = Substitute.For<ISession>();
    session.IsClosing.Returns(sessionIsClosing);

    using CancellationTokenSource cts = new();
    cts.Cancel();

    ImmediateBackgroundTaskScheduler scheduler = new(cts.Token);
    TestProtocolHandler handler = new(session, TimeSpan.FromMilliseconds(50), scheduler);

    handler.ScheduleBackgroundTask(static (_, ct) => ValueTask.FromException(new OperationCanceledException(ct)));

    if (sessionIsClosing)
        session.DidNotReceive().InitiateDisconnect(Arg.Any<DisconnectReason>(), Arg.Any<string>());
    else
        session.Received().InitiateDisconnect(DisconnectReason.BackgroundTaskFailure, Arg.Any<string>());
}

Fix this →

---

Low — GetAwaiter().GetResult() in test scheduler

ImmediateBackgroundTaskScheduler.TryScheduleTask blocks synchronously on an async method:

fulfillFunc(request, cancellationToken).GetAwaiter().GetResult();

The robustness rule says not to block on tasks inside async methods. This particular call is inside a synchronous method, so there is no deadlock risk in the current tests (the tasks complete immediately via ValueTask.FromException). It is still worth noting because any future test that introduces a genuinely async background task here would deadlock under a SynchronizationContext. A Task.Run(() => fulfillFunc(...)).GetAwaiter().GetResult() or restructuring to async would be safer long-term.

---

Note — IsClosing is read outside the session state lock

Session.IsClosing (line 80: State > SessionState.Initialized) reads the _state field, which is not marked volatile and is only mutated under _sessionStateLock. The read in the exception filter happens from a background thread without that lock. In practice this is benign because the state transition is monotonic (once closing, never returns to non-closing), so the worst case is a stale false read — triggering one extra InitiateDisconnect on a closing session, which the session itself guards against. Not a correctness issue.

---

Verdict

Severity	Count
Critical	0
High	0
Medium	1 (test parameterization)
Low	1

The core fix is correct and the tests verify both branches. The medium finding (test duplication rule) should be addressed before merge.

[LukaszRozmej]

please fix both claude issues

[mk0walsk]

@LukaszRozmej Updated, please re-check.

[LukaszRozmej]

@claude re-review

[claude]

Claude finished @LukaszRozmej's task in 1m 29s —— View job

---

PR Re-Review

• Read rule files
• Read changed files
• Check previous findings are resolved
• Post re-review

---

Summary

Both findings from the previous review have been addressed.

Medium — test parameterization: ✅ Resolved. The two duplicate test methods are now a single [TestCase(true)]/[TestCase(false)] parameterized method Operation_canceled_behavior_depends_on_session_closing, in line with the [TestCase] rule.

Low — GetAwaiter().GetResult() blocking: ✅ Resolved. ImmediateBackgroundTaskScheduler.TryScheduleTask now stores the returned Task in ScheduledTask and the test awaits it explicitly with await backgroundTaskScheduler.ScheduledTask. No synchronous blocking remains.

The implementation in BackgroundTaskSchedulerWrapper.cs is unchanged and correct. The when (cancellationToken.IsCancellationRequested && handler.Session.IsClosing) guard is sound, and the test exercises both branches cleanly.

The earlier Note about IsClosing being read outside the session lock remains a benign observation (monotonic state, worst case is one spurious disconnect attempt that the session guards against). Not a correctness issue.

---

Verdict

Severity	Count
Critical	0
High	0
Medium	0
Low	0

Clean. Ready to merge.