Complete the unregister before shutting the transport down

Network/SipClient.cs

@@ -35,6 +35,9 @@
     private const int DefaultRegistrationExpirySeconds = 60;
     private const int BlindTransferTimeoutSeconds = 10;
 
+    /// <summary>How long <see cref="Shutdown"/> waits for the registrar to confirm the unregister.</summary>
+    internal static readonly TimeSpan UnregisterTimeout = TimeSpan.FromSeconds(3);
+
     private readonly string _sipUsername;
     private readonly string _sipPassword;
     private readonly string _sipServer;
@@ -73,8 +76,8 @@
     public event Action<SipClient>? CallAnswer;
     public event Action<SipClient>? CallEnded;
     public event Action<SipClient, string>? StatusMessage;
     public event Action<SipClient>? RemotePutOnHold;
     public event Action<SipClient>? RemoteTookOffHold;
     public event Action<SipClient, Exception>? ErrorOccurred;
     public event Action<SipClient>? RegistrationStatusChanged;
     public event Action<SipClient, TimeSpan>? CallDurationUpdated;
@@ -136,7 +139,7 @@
     public string Username => _sipUsername;
     public string Server => _sipServer;
 
     public SipClient(
         SIPTransport sipTransport,
         SipConfig sipSettings,
         int registrationExpirySeconds = DefaultRegistrationExpirySeconds,
@@ -727,11 +730,16 @@
             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 @@
         }
     }
 
+    /// <summary>
+    /// Sends the unregister (REGISTER with Expires 0) and waits up to
+    /// <see cref="UnregisterTimeout"/> for the registrar to confirm it. The unregister is usually
+    /// challenged (401, then an authenticated retry), so shutting the transport down right after
+    /// <see cref="SIPRegistrationUserAgent.Stop"/> used to lose it. When not registered, the agent
+    /// is just stopped and nothing is sent.
+    /// </summary>
+    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

tests/SipBotLib.Tests/ShutdownUnregisterTests.cs

@@ -0,0 +1,76 @@
+using System.Diagnostics;
+using SipBot;
+using Xunit;
+using static SipBotLib.Tests.FakeRegistrar;
+
+namespace SipBotLib.Tests;
+
+/// <summary>
+/// Shutdown unregisters (REGISTER Expires 0, answering a digest challenge if one comes) before the
+/// transport closes, waits at most <see cref="SipClient.UnregisterTimeout"/> for it, and schedules
+/// no reconnection afterwards.
+/// </summary>
+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}");
+    }
+}