From 0c36b2664cdad870e71ea5ce0622d5f76fc5ba9b Mon Sep 17 00:00:00 2001 From: calebtt Date: Fri, 25 Sep 2026 20:01:37 -0500 Subject: [PATCH] Complete the unregister before shutting the transport down SipClient.Shutdown() called SIPRegistrationUserAgent.Stop(), which queues the unregister (REGISTER with Expires 0) on a thread-pool thread, and then shut the transport down straight away. The unregister is usually challenged, so it rarely completed: the registrar kept a dead contact until it expired and forked calls to it. Shutdown now sends the unregister and waits up to 3 s for the registrar to confirm it (RegistrationRemoved, or a failure) before shutting the transport down. When the client is not registered it stops the agent and sends nothing. RegistrationRemoved no longer leads to a reconnect once shutdown has begun (ScheduleReconnection checks _isShutdown). Closes #16 Co-Authored-By: Claude Opus 5.5 --- Network/SipClient.cs | 62 ++++++++++++++- .../ShutdownUnregisterTests.cs | 76 +++++++++++++++++++ 2 files changed, 134 insertions(+), 4 deletions(-) create mode 100644 tests/SipBotLib.Tests/ShutdownUnregisterTests.cs diff --git a/Network/SipClient.cs b/Network/SipClient.cs index d44e72d..2f42706 100644 --- a/Network/SipClient.cs +++ b/Network/SipClient.cs @@ -35,6 +35,9 @@ public class SipClient : IDisposable private const int DefaultRegistrationExpirySeconds = 60; private const int BlindTransferTimeoutSeconds = 10; + /// How long waits for the registrar to confirm the unregister. + internal static readonly TimeSpan UnregisterTimeout = TimeSpan.FromSeconds(3); + private readonly string _sipUsername; private readonly string _sipPassword; private readonly string _sipServer; @@ -727,11 +730,16 @@ public void Shutdown() mediaSessionToDispose?.Close("Shutdown"); mediaSessionToDispose?.Dispose(); - _registrationAgent?.Stop(); - StopHealthMonitoring(); - StopReconnectionTimer(); - + lock (_lockObject) + { + StopReconnectionTimer(); + } + + // Unregister before the transport goes away; otherwise the registrar keeps a dead + // contact until it expires and forks calls to it. + UnregisterBeforeShutdown(); + _sipTransport.Shutdown(); IncrementMetric("shutdowns"); @@ -744,6 +752,52 @@ public void Shutdown() } } + /// + /// Sends the unregister (REGISTER with Expires 0) and waits up to + /// for the registrar to confirm it. The unregister is usually + /// challenged (401, then an authenticated retry), so shutting the transport down right after + /// used to lose it. When not registered, the agent + /// is just stopped and nothing is sent. + /// + private void UnregisterBeforeShutdown() + { + SIPRegistrationUserAgent agent; + bool registered; + lock (_lockObject) + { + agent = _registrationAgent; + registered = _isRegistered; + } + + if (!registered) + { + agent.Stop(sendZeroExpiryRegister: false); + return; + } + + using var done = new ManualResetEventSlim(false); + void Removed(SIPURI uri, SIPResponse resp) => done.Set(); + void Failed(SIPURI uri, SIPResponse resp, string error) => done.Set(); + agent.RegistrationRemoved += Removed; + agent.RegistrationFailed += Failed; + agent.RegistrationTemporaryFailure += Failed; + try + { + agent.Stop(sendZeroExpiryRegister: true); + if (done.Wait(UnregisterTimeout)) + Log.Information("Unregistered {User}@{Server}", _sipUsername, _sipServer); + else + Log.Warning("Unregister of {User}@{Server} not confirmed within {Timeout}; the registrar keeps the contact until it expires", + _sipUsername, _sipServer, UnregisterTimeout); + } + finally + { + agent.RegistrationRemoved -= Removed; + agent.RegistrationFailed -= Failed; + agent.RegistrationTemporaryFailure -= Failed; + } + } + private MediaEndPoints CreateMediaEndPoints(IAudioSink audioSink, IAudioSource audioSource) { var mediaEndPoints = new MediaEndPoints diff --git a/tests/SipBotLib.Tests/ShutdownUnregisterTests.cs b/tests/SipBotLib.Tests/ShutdownUnregisterTests.cs new file mode 100644 index 0000000..bf39980 --- /dev/null +++ b/tests/SipBotLib.Tests/ShutdownUnregisterTests.cs @@ -0,0 +1,76 @@ +using System.Diagnostics; +using SipBot; +using Xunit; +using static SipBotLib.Tests.FakeRegistrar; + +namespace SipBotLib.Tests; + +/// +/// Shutdown unregisters (REGISTER Expires 0, answering a digest challenge if one comes) before the +/// transport closes, waits at most for it, and schedules +/// no reconnection afterwards. +/// +public class ShutdownUnregisterTests +{ + [Theory] + [InlineData(false)] + [InlineData(true)] + public async Task Registered_client_unregisters_before_the_transport_closes(bool challengeFirst) + { + using var h = new RegistrationTestHarness(); + h.Registrar.ChallengeFirst = challengeFirst; + h.Registrar.Script = Reply.Ok; + h.Client.StartRegistration(); + Assert.True(await h.WaitForState(RegistrationState.Registered, TimeSpan.FromSeconds(5))); + + var elapsed = Stopwatch.StartNew(); + h.Client.Dispose(); + elapsed.Stop(); + + var last = h.Registrar.Registers[^1]; + Assert.Equal(0, last.Expires); + Assert.Equal(challengeFirst, last.Authenticated); + Assert.True(elapsed.Elapsed < TimeSpan.FromSeconds(2), $"shutdown took {elapsed.Elapsed}"); + + // RegistrationRemoved must not trigger a reconnect on a client that is shutting down. + int count = h.Registrar.Count; + await Task.Delay(1000); + Assert.Equal(count, h.Registrar.Count); + } + + [Fact] + public async Task Unconfirmed_unregister_is_bounded_by_the_timeout() + { + using var h = new RegistrationTestHarness(); + h.Registrar.Script = Reply.Ok; + h.Client.StartRegistration(); + Assert.True(await h.WaitForState(RegistrationState.Registered, TimeSpan.FromSeconds(5))); + h.Registrar.Script = Reply.Drop; + + var elapsed = Stopwatch.StartNew(); + h.Client.Dispose(); + elapsed.Stop(); + + Assert.Equal(0, h.Registrar.Registers[^1].Expires); + Assert.InRange(elapsed.Elapsed, SipClient.UnregisterTimeout - TimeSpan.FromMilliseconds(300), + SipClient.UnregisterTimeout + TimeSpan.FromSeconds(2)); + } + + [Fact] + public async Task Unregistered_client_sends_nothing_on_shutdown() + { + using var h = new RegistrationTestHarness(); + h.Registrar.Script = Reply.Forbidden; + h.Client.StartRegistration(); + Assert.True(await h.WaitForState(RegistrationState.HardFailure, TimeSpan.FromSeconds(5))); + int count = h.Registrar.Count; + + var elapsed = Stopwatch.StartNew(); + h.Client.Dispose(); + elapsed.Stop(); + + await Task.Delay(500); + Assert.Equal(count, h.Registrar.Count); + Assert.True(elapsed.Elapsed < TimeSpan.FromSeconds(1), $"shutdown took {elapsed.Elapsed}"); + } +}