From e936ab4646ee049a5a5bb27106aaade0a927973d Mon Sep 17 00:00:00 2001 From: Jaap-Jan de Wit | DodoTech Date: Sun, 9 Aug 2026 13:01:14 +0200 Subject: [PATCH] Announce a session's end when it is actually over, and for closes too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 10 +++ docs/manual-checks.md | 10 ++- .../Platform/SessionKeepAlive.cs | 7 +- .../ViewModels/MainWindowViewModel.cs | 6 +- .../TerminalWorkspace.cs | 87 +++++++++++++++---- .../TerminalWorkspaceTests.cs | 45 ++++++++-- 6 files changed, 132 insertions(+), 33 deletions(-) diff --git a/docs/android-port.md b/docs/android-port.md index 7724b45..cd6021e 100644 --- a/docs/android-port.md +++ b/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 diff --git a/docs/manual-checks.md b/docs/manual-checks.md index 6ea93fd..1d07b81 100644 --- a/docs/manual-checks.md +++ b/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 diff --git a/src/DodoSSH.Client.Android/Platform/SessionKeepAlive.cs b/src/DodoSSH.Client.Android/Platform/SessionKeepAlive.cs index 1f97d3b..374d6bc 100644 --- a/src/DodoSSH.Client.Android/Platform/SessionKeepAlive.cs +++ b/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 - // a binder call and needs no particular thread. Nothing here touches the interface. + // Raised on whatever thread the workspace announced from — a continuation of the ended run, or the + // 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; } diff --git a/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs b/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs index 3eaae25..e81699f 100644 --- a/src/DodoSSH.Client.Shell/ViewModels/MainWindowViewModel.cs +++ b/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 - // mark dead and does nothing. The alternative ordering leaves a window in which a tab that is on its - // way out is repainted as disconnected. + // Removed first, so the workspace's SessionEnded — announced once the close below has fully drained + // — finds no tab to mark dead and does nothing here. The alternative ordering leaves a window in + // which a tab that is on its way out is repainted as disconnected. var index = Tabs.IndexOf(tab); Tabs.Remove(tab); diff --git a/src/DodoSSH.Client.Terminal/TerminalWorkspace.cs b/src/DodoSSH.Client.Terminal/TerminalWorkspace.cs index 098b6b4..ef97e06 100644 --- a/src/DodoSSH.Client.Terminal/TerminalWorkspace.cs +++ b/src/DodoSSH.Client.Terminal/TerminalWorkspace.cs @@ -257,7 +257,8 @@ public sealed class TerminalWorkspace : IAsyncDisposable } /// - /// 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. /// /// /// @@ -268,13 +269,23 @@ public sealed class TerminalWorkspace : IAsyncDisposable /// the half of the interface Avalonia draws. /// /// - /// Raised on whatever thread the pump finished on, which is a thread-pool thread. A handler that - /// touches an observable collection has to marshal; this type has no toolkit to do it with, which is - /// exactly why it does not try. + /// Raised only after the session's run task has completed, and that ordering is load-bearing. + /// It used to fire from inside the run's own finally block, where the task is by definition not yet + /// complete — so a handler reading 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 . 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. /// /// - /// Not raised by . That path already has a caller who knows the session is - /// going, and telling it what it just asked for is how a tab close turns into a second tab close. + /// Raised by too, which reverses a recorded decision. The old + /// 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: is tearing the subscribers down with the + /// sessions, and news nobody is left to hear is not news. /// /// public event EventHandler? 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)); + } } /// @@ -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 - // entry before it disposes the pump, so a tab the user closed does not come back as news. - bool announce; + /// Announces a session's end once its run task has actually completed. + /// + /// + /// A continuation rather than a line in '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 , 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. + /// + /// + /// The containment check keeps the deliberate paths out of this route: + /// removes the entry before it disposes the pump, and makes its own announcement after its own drain. + /// + /// + 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) - { - announce = sessions.ContainsKey(sessionId); - } + bool announce; - if (announce) - { - SessionEnded?.Invoke(this, new TerminalSessionEndedEventArgs(sessionId)); - } + lock (sessionGate) + { + announce = sessions.ContainsKey(sessionId); + } + + if (announce) + { + SessionEnded?.Invoke(this, new TerminalSessionEndedEventArgs(sessionId)); } } diff --git a/tests/DodoSSH.Client.Terminal.Tests/TerminalWorkspaceTests.cs b/tests/DodoSSH.Client.Terminal.Tests/TerminalWorkspaceTests.cs index 52dca61..bed2e44 100644 --- a/tests/DodoSSH.Client.Terminal.Tests/TerminalWorkspaceTests.cs +++ b/tests/DodoSSH.Client.Terminal.Tests/TerminalWorkspaceTests.cs @@ -217,12 +217,15 @@ public sealed class TerminalWorkspaceTests } /// - /// The event the tab strip listens to, so a dot can go out the moment a shell exits rather than at the - /// next thing that happens to repaint. Raised only when the session ended on its own: a tab the user - /// closed has a caller who already knows, and telling it would turn one close into two. + /// The event the tab strip and the phone's keep-alive listen to, so a dot can go out — and a foreground + /// notification can come down — the moment a shell exits rather than at the next thing that happens to + /// 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 + /// LiveSessionCount 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. /// [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(); + 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"); + } } - /// + /// + /// 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. + /// [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"); } /// /// 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. /// [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 ----