Announce a session's end when it is actually over, and for closes too

The phone's notification kept saying '1 shell connected' after the
shell was gone, and both close routes were at fault. A shell exiting on
its own raised SessionEnded from inside its run's finally block — where
the run task is by definition not yet complete, so the LiveSessionCount
the keep-alive reads still counted the dead shell, and nothing fired
later to correct it. A tab closed by hand announced nothing at all, by
a recorded decision that assumed every subscriber was the closer; the
keep-alive is not, and a close it never heard about left the
notification claiming a shell over nothing.

The end is now announced from a continuation after the run completes,
and CloseSessionAsync announces after its own drain — every subscriber
was already a reconcile-to-reality handler, so the echo the old remark
feared costs nothing. Shutdown stays silent: it is dismantling the
subscribers along with the sessions.

docs/android-port.md

@@ -348,6 +348,16 @@ runtime or the receipt is silently invisible — the service still runs, but not
 to show, at most once, with no result read back: a refusal costs the notification and nothing else, which is
 what the manifest's own comment on the permission says.
 
+**And a fourth correction, found by the notification refusing to come down.** "1 shell connected" outlived
+the shell, both ways a shell can close. A shell exiting on its own announced `SessionEnded` from inside its
+run's own finally block — where the run task is by definition not yet complete, so the
+`LiveSessionCount` the keep-alive reads from that event still counted the session that had just ended, and
+nothing fired afterwards to correct it. A tab closed by hand announced nothing at all, by a recorded
+decision that assumed every subscriber was the closer. Both reversed in `TerminalWorkspace`: the end is now
+announced from a continuation after the run has actually completed, and `CloseSessionAsync` announces too,
+after its own drain — the event's remark carries the reversal, and `SessionEnded`'s subscribers were all
+already "reconcile to reality" handlers for which a second announcement is harmless.
+
 ### Phone first
 
 About 360dp wide. The tablet route was cheaper — a landscape tablet is close to the existing 880×560 minimum

docs/manual-checks.md

@@ -2130,13 +2130,19 @@ foreground service is protecting), wait thirty seconds with the shell doing noth
 **Pass:** the notification stayed up the whole time, and the shell is exactly where it was — same scrollback,
 same prompt — with typing reaching the host immediately. Exit the shell.
 
-**Pass:** the notification goes with it, once nothing else is open.
+**Pass:** the notification goes with it, once nothing else is open. Open another shell and close it from the
+shells strip's ✕ instead of exiting — the notification comes down for that route too, which is the route
+that used to leave it up: a deliberate close announced nothing to the keep-alive at all, and a shell exiting
+on its own was announced while the count still included it.
 
 **Failure means:** an upload that stalls with the screen off is the count not reaching
 `SessionForegroundService`, and Android has stopped the process mid-transfer. A notification left up
 afterwards is `ActivityChanged` not being subscribed — the other end of the same wire. A shell that has
 disconnected on return is `MainWindowViewModel.TerminalSessionOpened` never reaching `SessionKeepAlive` — the
-service only ever heard about a shell *ending*, so it never came up for one in the first place.
+service only ever heard about a shell *ending*, so it never came up for one in the first place. A
+notification still saying "1 shell connected" after the shell is gone — by either route — is
+`TerminalWorkspace.SessionEnded` firing before the run completed, or a close not announcing; see
+`AnnounceEndedAsync` and the event's own remark.
 
 ### 14.6a A Files connection with nothing moving still survives backgrounding
 

src/DodoSSH.Client.Android/Platform/SessionKeepAlive.cs

@@ -46,8 +46,11 @@ internal sealed class SessionKeepAlive : IDisposable
         this.activeTransfers = activeTransfers;
         this.holdsFileSession = holdsFileSession;
 
-        // Raised on whatever thread the pump unwound on, which is fine: starting and stopping a service is
+        // Raised on whatever thread the workspace announced from — a continuation of the ended run, or the
-        // a binder call and needs no particular thread. Nothing here touches the interface.
+        // closer's own — which is fine: starting and stopping a service is a binder call and needs no
+        // particular thread. Nothing here touches the interface. That the announcement waits for the run to
+        // actually complete, and comes for deliberate closes too, is what makes reading LiveSessionCount
+        // from it honest — the event's own remark carries the stuck notification that taught us both.
         workspace.SessionEnded += OnSessionEnded;
     }
 

src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs

@@ -1936,9 +1936,9 @@ internal sealed partial class MainWindowViewModel : ObservableObject, IAsyncDisp
             return;
         }
 
-        // Removed first, so the workspace's SessionEnded — which fires as the pump unwinds — finds no tab to
+        // Removed first, so the workspace's SessionEnded — announced once the close below has fully drained
-        // mark dead and does nothing. The alternative ordering leaves a window in which a tab that is on its
+        // — finds no tab to mark dead and does nothing here. The alternative ordering leaves a window in
-        // way out is repainted as disconnected.
+        // which a tab that is on its way out is repainted as disconnected.
         var index = Tabs.IndexOf(tab);
         Tabs.Remove(tab);
 

src/DodoSSH.Client.Terminal/TerminalWorkspace.cs

@@ -257,7 +257,8 @@ public sealed class TerminalWorkspace : IAsyncDisposable
     }
 
     /// <summary>
-    /// Raised with the session id when a shell ends on its own.
+    /// Raised with the session id once a session is over — its shell having ended on its own, or a
+    /// deliberate close having fully drained.
     /// </summary>
     /// <remarks>
     /// <para>
@@ -268,13 +269,23 @@ public sealed class TerminalWorkspace : IAsyncDisposable
     /// the half of the interface Avalonia draws.
     /// </para>
     /// <para>
-    /// <b>Raised on whatever thread the pump finished on</b>, which is a thread-pool thread. A handler that
+    /// <b>Raised only after the session's run task has completed, and that ordering is load-bearing.</b>
-    /// touches an observable collection has to marshal; this type has no toolkit to do it with, which is
+    /// It used to fire from inside the run's own finally block, where the task is by definition not yet
-    /// exactly why it does not try.
+    /// complete — so a handler reading <see cref="LiveSessionCount"/> still counted the session that had
+    /// just ended, which is how the phone's foreground notification went on saying "1 shell connected"
+    /// over nothing. See <see cref="AnnounceEndedAsync"/>. Raised on a thread-pool continuation, or on the
+    /// closer's own thread; a handler that touches an observable collection has to marshal either way.
     /// </para>
     /// <para>
-    /// Not raised by <see cref="CloseSessionAsync"/>. That path already has a caller who knows the session is
+    /// <b>Raised by <see cref="CloseSessionAsync"/> too, which reverses a recorded decision.</b> The old
-    /// going, and telling it what it just asked for is how a tab close turns into a second tab close.
+    /// reasoning — the caller asked, so telling it is an echo — assumed every subscriber was the caller.
+    /// The phone's keep-alive is not: it hears this event to reconcile a notification with reality, and a
+    /// close that announced nothing left that notification claiming a shell that was gone. Every subscriber
+    /// treats the event as "reconcile" rather than "act" — a tab is marked dead if it is still there and
+    /// skipped if it is not — so a second announcement for a session that already announced its own end
+    /// (closing the tab of a shell that exited earlier) is deliberate and harmless. Shutdown is the one
+    /// close that stays silent: <see cref="DisposeAsync"/> is tearing the subscribers down with the
+    /// sessions, and news nobody is left to hear is not news.
     /// </para>
     /// </remarks>
     public event EventHandler<TerminalSessionEndedEventArgs>? SessionEnded;
@@ -403,6 +414,10 @@ public sealed class TerminalWorkspace : IAsyncDisposable
             sessions[sessionId] = new LiveSession(connection, pump, run);
         }
 
+        // The announcement's own continuation — see AnnounceEndedAsync. Started after the entry is stored,
+        // so the containment check inside it can never run against a dictionary the session had not reached.
+        _ = AnnounceEndedAsync(sessionId, run);
+
         return sessionId;
     }
 
@@ -574,6 +589,14 @@ public sealed class TerminalWorkspace : IAsyncDisposable
         {
             // Expected on the ordinary path: disposing the pump cancels its run.
         }
+
+        // After the drain, so a handler reading LiveSessionCount sees this session already gone — the
+        // event's own remark carries why a deliberate close is announced at all, and why shutdown is not:
+        // DisposeAsync sets the flag before its closing loop, and is dismantling every subscriber anyway.
+        if (Volatile.Read(ref disposed) == 0)
+        {
+            SessionEnded?.Invoke(this, new TerminalSessionEndedEventArgs(sessionId));
+        }
     }
 
     /// <inheritdoc />
@@ -694,21 +717,51 @@ public sealed class TerminalWorkspace : IAsyncDisposable
             // the pump unwinding, which is why CloseSessionAsync needs no call of its own — and why this
             // must not do any work: it is running on a thread-pool thread inside DisposeAsync's loop when
             // the application is closing.
+            //
+            // SessionEnded is deliberately NOT raised from here, and it used to be — see
+            // AnnounceEndedAsync for what was wrong with that.
             ConnectionLog?.Closed(sessionId, clock.GetUtcNow());
+        }
+    }
 
-            // Only when the session is still one this workspace knows about. CloseSessionAsync removes the
+    /// <summary>Announces a session's end once its run task has actually completed.</summary>
-            // entry before it disposes the pump, so a tab the user closed does not come back as news.
+    /// <remarks>
-            bool announce;
+    /// <para>
+    /// A continuation rather than a line in <see cref="RunSessionAsync"/>'s finally, and the difference is
+    /// what a handler sees. Inside that finally the run task is not yet complete — a finally is part of the
+    /// task — so <see cref="LiveSessionCount"/>, which counts incomplete runs, still included the session
+    /// that had just ended. The phone's keep-alive answers this event by reading exactly that count, and
+    /// reconciled its foreground notification to "1 shell connected" over a shell that was gone, with
+    /// nothing left to fire afterwards and correct it. By the time an await on the run resumes, the task is
+    /// complete and the count is honest.
+    /// </para>
+    /// <para>
+    /// The containment check keeps the deliberate paths out of this route: <see cref="CloseSessionAsync"/>
+    /// removes the entry before it disposes the pump, and makes its own announcement after its own drain.
+    /// </para>
+    /// </remarks>
+    private async Task AnnounceEndedAsync(uint sessionId, Task run)
+    {
+        try
+        {
+            await run.ConfigureAwait(false);
+        }
+        catch (Exception exception) when (exception is not OutOfMemoryException)
+        {
+            // The run's faults belong to whoever drains it — CloseSessionAsync, on the deliberate path.
+            // This continuation cares only that the run is over, however it got there.
+        }
 
-            lock (sessionGate)
+        bool announce;
-            {
-                announce = sessions.ContainsKey(sessionId);
-            }
 
-            if (announce)
+        lock (sessionGate)
-            {
+        {
-                SessionEnded?.Invoke(this, new TerminalSessionEndedEventArgs(sessionId));
+            announce = sessions.ContainsKey(sessionId);
-            }
+        }
+
+        if (announce)
+        {
+            SessionEnded?.Invoke(this, new TerminalSessionEndedEventArgs(sessionId));
         }
     }
 

tests/DodoSSH.Client.Terminal.Tests/TerminalWorkspaceTests.cs

@@ -217,12 +217,15 @@ public sealed class TerminalWorkspaceTests
     }
 
     /// <remarks>
-    /// The event the tab strip listens to, so a dot can go out the moment a shell exits rather than at the
+    /// The event the tab strip and the phone's keep-alive listen to, so a dot can go out — and a foreground
-    /// next thing that happens to repaint. Raised only when the session ended on its own: a tab the user
+    /// notification can come down — the moment a shell exits rather than at the next thing that happens to
-    /// closed has a caller who already knows, and telling it would turn one close into two.
+    /// repaint. The count captured inside the handler is the sharper half of this test: the announcement
+    /// used to fire from inside the run's own finally block, where the run task is not yet complete, so
+    /// <c>LiveSessionCount</c> read from the handler still said 1 — and the phone's notification went on
+    /// claiming a shell that was gone, with nothing left to fire and correct it.
     /// </remarks>
     [Fact]
-    public async Task ASessionEndingOnItsOwnIsAnnounced()
+    public async Task ASessionEndingOnItsOwnIsAnnounced_AfterTheCountStoppedIncludingIt()
     {
         // A shell with no output to give: its first read returns 0, which is a remote closing the channel,
         // so the pump finishes with nobody asking it to.
@@ -231,10 +234,12 @@ public sealed class TerminalWorkspaceTests
         await using var workspace = CreateWorkspace(connections);
 
         var ended = new List<uint>();
+        var liveAtAnnouncement = -1;
         workspace.SessionEnded += (_, e) =>
         {
             lock (ended)
             {
+                liveAtAnnouncement = workspace.LiveSessionCount;
                 ended.Add(e.SessionId);
             }
         };
@@ -249,18 +254,33 @@ public sealed class TerminalWorkspaceTests
                 return ended.Contains(sessionId);
             }
         });
+
+        lock (ended)
+        {
+            liveAtAnnouncement.ShouldBe(0, "the announcement must wait for the run to actually complete");
+        }
     }
 
-    /// <inheritdoc cref="ASessionEndingOnItsOwnIsAnnounced" />
+    /// <remarks>
+    /// The reversal of a recorded decision, and the event's own remark carries why: a close used to be
+    /// announced to nobody, on the theory that the caller already knew — but the phone's keep-alive is not
+    /// the caller, and a close it never heard about left the foreground notification claiming a shell that
+    /// was gone. Announced once, after the drain, so the count a handler reads is already honest.
+    /// </remarks>
     [Fact]
-    public async Task ClosingASessionIsNotAnnouncedBack()
+    public async Task ClosingASessionIsAnnounced_OnceItHasDrained()
     {
         var connections = new FakeConnectionFactory();
 
         await using var workspace = CreateWorkspace(connections);
 
         var announcements = 0;
-        workspace.SessionEnded += (_, _) => Interlocked.Increment(ref announcements);
+        var liveAtAnnouncement = -1;
+        workspace.SessionEnded += (_, _) =>
+        {
+            liveAtAnnouncement = workspace.LiveSessionCount;
+            Interlocked.Increment(ref announcements);
+        };
 
         var sessionId = await workspace.OpenSessionAsync(
             Request(), TerminalSize.Default, TestContext.Current.CancellationToken);
@@ -268,12 +288,15 @@ public sealed class TerminalWorkspaceTests
         await workspace.CloseSessionAsync(sessionId);
 
         Volatile.Read(ref announcements)
-            .ShouldBe(0, "a close the caller asked for is not news to report back to it");
+            .ShouldBe(1, "a close is news to the keep-alive even though it is an echo to the closer");
+        liveAtAnnouncement.ShouldBe(0, "announced after the drain, so the count already excludes it");
     }
 
     /// <remarks>
     /// Disposal is the other path that closes sessions, because it is process shutdown. Asserted so
-    /// that the SSH connections are known to be released rather than assumed to be.
+    /// that the SSH connections are known to be released rather than assumed to be — and that these closes,
+    /// unlike a deliberate one, are announced to nobody: shutdown is dismantling every subscriber along
+    /// with the sessions, and news nobody is left to hear is not news.
     /// </remarks>
     [Fact]
     public async Task DisposingTheWorkspaceClosesEverySession()
@@ -282,6 +305,9 @@ public sealed class TerminalWorkspaceTests
 
         var workspace = CreateWorkspace(connections);
 
+        var announcements = 0;
+        workspace.SessionEnded += (_, _) => Interlocked.Increment(ref announcements);
+
         await workspace.OpenSessionAsync(
             Request(), TerminalSize.Default, TestContext.Current.CancellationToken);
         await workspace.OpenSessionAsync(
@@ -292,6 +318,7 @@ public sealed class TerminalWorkspaceTests
         workspace.LiveSessionCount.ShouldBe(0);
         connections.Connections.Count.ShouldBe(2);
         connections.Connections.ShouldAllBe(connection => connection.IsDisposed);
+        Volatile.Read(ref announcements).ShouldBe(0, "shutdown closes are not announced");
     }
 
     // ---- Reattach ----