fix(daemon): surface refused close of last tab instead of silent no-op
- commit
- 601c376de9843fb350517c616ae6694b1ec166a9
- author
- brice · · 2026-07-20 18:33 UTC
- parent
- 650884a
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
2 files · +81 -1
M internal/usecase/daemon/session.go +6 -1
| @@ -632,8 +632,13 @@ func (d *Daemon) closeTab(sess *session, tb *tab, repaint bool) { | |||
| 632 | 632 | name := sess.name | |
| 633 | 633 | sess.mu.Unlock() | |
| 634 | 634 | d.mu.Unlock() | |
| 635 | + | if err := d.killSession(sess, ports.ReasonSessionKilled, false); err != nil { | |
| 636 | + | d.log.Warn("closing last tab failed", "session", name, "err", err) | |
| 637 | + | d.reportError(sess, domain.UserErr(domain.NoticeSnapshotSaturated, | |
| 638 | + | "couldn't close tab: session state not yet saved; try again", err)) | |
| 639 | + | return | |
| 640 | + | } | |
| 635 | 641 | d.log.Info("tab closed", "session", name, "last", true) | |
| 636 | - | _ = d.killSession(sess, ports.ReasonSessionKilled, false) | |
| 637 | 642 | return | |
| 638 | 643 | } | |
| 639 | 644 | ringing := tb.attention | |
| @@ -632,8 +632,13 @@ func (d *Daemon) closeTab(sess *session, tb *tab, repaint bool) { | |||||
| 632 | name := sess.name | 632 | name := sess.name | ||
| 633 | sess.mu.Unlock() | 633 | sess.mu.Unlock() | ||
| 634 | d.mu.Unlock() | 634 | d.mu.Unlock() | ||
| 635 | + | if err := d.killSession(sess, ports.ReasonSessionKilled, false); err != nil { | |||
| 636 | + | d.log.Warn("closing last tab failed", "session", name, "err", err) | |||
| 637 | + | d.reportError(sess, domain.UserErr(domain.NoticeSnapshotSaturated, | |||
| 638 | + | "couldn't close tab: session state not yet saved; try again", err)) | |||
| 639 | + | return | |||
| 640 | + | } | |||
| 635 | d.log.Info("tab closed", "session", name, "last", true) | 641 | d.log.Info("tab closed", "session", name, "last", true) | ||
| 636 | - | _ = d.killSession(sess, ports.ReasonSessionKilled, false) | |||
| 637 | return | 642 | return | ||
| 638 | } | 643 | } | ||
| 639 | ringing := tb.attention | 644 | ringing := tb.attention | ||
M internal/usecase/daemon/session_test.go +75 -0
| @@ -3,6 +3,7 @@ package daemon | |||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | 5 | "errors" | |
| 6 | + | "fmt" | |
| 6 | 7 | "io" | |
| 7 | 8 | "log/slog" | |
| 8 | 9 | "math" | |
| @@ -20,6 +21,7 @@ import ( | |||
| 20 | 21 | "github.com/bnema/vev/internal/ports" | |
| 21 | 22 | portsmocks "github.com/bnema/vev/internal/ports/mocks" | |
| 22 | 23 | "github.com/bnema/vev/internal/usecase/picker" | |
| 24 | + | snapcodec "github.com/bnema/vev/internal/usecase/snapshot" | |
| 23 | 25 | ) | |
| 24 | 26 | ||
| 25 | 27 | // --- test doubles ----------------------------------------------------------- | |
| @@ -972,6 +974,79 @@ func TestCloseActiveTabActivatesDestinationFloatingPane(t *testing.T) { | |||
| 972 | 974 | requireFloatingInitialized(t, first) | |
| 973 | 975 | } | |
| 974 | 976 | ||
| 977 | + | // TestCloseLastTabSaturatedSnapshotSurfacesNotice proves that when closing the | |
| 978 | + | // last tab of a named session is refused because the snapshot worker cannot | |
| 979 | + | // retain the final state, the session survives for a retry and the still | |
| 980 | + | // attached client is told why, instead of the close silently no-oping. | |
| 981 | + | // | |
| 982 | + | // The snapshot worker is saturated deterministically with the same fixture as | |
| 983 | + | // the kill-session ordering tests: one capture blocked in marshal, one in the | |
| 984 | + | // bounded queue, and the terminal-retention fallback filled to capacity. No | |
| 985 | + | // assertion runs on the worker goroutine; the marshal gate is released only on | |
| 986 | + | // cleanup, after every assertion has run on the test goroutine. | |
| 987 | + | func TestCloseLastTabSaturatedSnapshotSurfacesNotice(t *testing.T) { | |
| 988 | + | store := discardSnapshotStore{} | |
| 989 | + | d := newTestDaemon(t, portsmocks.NewMockPTYFactory(t), stubClock{}) | |
| 990 | + | WithSnapshotStore(store)(d) | |
| 991 | + | startSnapshotEncodeWorker(t, d) | |
| 992 | + | ||
| 993 | + | entered := make(chan struct{}) | |
| 994 | + | release := make(chan struct{}) | |
| 995 | + | // Release the sole blocked marshal on the test goroutine before the worker | |
| 996 | + | // is stopped in cleanup, so teardown never waits on the gate. | |
| 997 | + | defer close(release) | |
| 998 | + | var calls atomic.Int32 | |
| 999 | + | d.snapshotMarshal = func(s snapcodec.Session) ([]byte, error) { | |
| 1000 | + | if calls.Add(1) == 1 { | |
| 1001 | + | close(entered) | |
| 1002 | + | <-release | |
| 1003 | + | } | |
| 1004 | + | return snapcodec.Marshal(s) | |
| 1005 | + | } | |
| 1006 | + | ||
| 1007 | + | active := newSnapshotTestSession(t, "active", false, "/work") | |
| 1008 | + | queued := newSnapshotTestSession(t, "queued", false, "/work") | |
| 1009 | + | markSnapshotDirty(active) | |
| 1010 | + | markSnapshotDirty(queued) | |
| 1011 | + | require.True(t, d.scheduleSnapshot(active)) | |
| 1012 | + | <-entered // the worker is now parked inside marshal for active | |
| 1013 | + | require.True(t, d.scheduleSnapshot(queued), "one capture waits in the bounded queue") | |
| 1014 | + | for i := range snapshotFinalQueueCapacity { | |
| 1015 | + | s := newSnapshotTestSession(t, fmt.Sprintf("terminal-%d", i), false, "/work") | |
| 1016 | + | markSnapshotDirty(s) | |
| 1017 | + | require.True(t, d.scheduleFinalSnapshot(s), "fill the terminal-retention fallback to capacity") | |
| 1018 | + | } | |
| 1019 | + | ||
| 1020 | + | // A single-tab named session whose client is still attached. | |
| 1021 | + | target := newSnapshotTestSession(t, "target", false, "/work") | |
| 1022 | + | tr, _ := newCapturingTransport(t) | |
| 1023 | + | ac := &attachedClient{tr: tr, output: newOutputStateStream(), size: domain.Size{Cols: 80, Rows: 24}} | |
| 1024 | + | ac.initOverlays() | |
| 1025 | + | ac.setSession(target) | |
| 1026 | + | target.client = ac | |
| 1027 | + | d.mu.Lock() | |
| 1028 | + | d.sessions[target.id] = target | |
| 1029 | + | d.mu.Unlock() | |
| 1030 | + | require.Len(t, target.tabs, 1) | |
| 1031 | + | ||
| 1032 | + | d.closeTab(target, target.tabs[0], false) | |
| 1033 | + | ||
| 1034 | + | // The refused close retains the session for a retry. | |
| 1035 | + | d.mu.Lock() | |
| 1036 | + | require.Same(t, target, d.sessions[target.id], "a refused close of the last tab must retain the session") | |
| 1037 | + | d.mu.Unlock() | |
| 1038 | + | ||
| 1039 | + | // The refusal is recorded in history and shown on the still-attached client. | |
| 1040 | + | hist := d.notices.history() | |
| 1041 | + | require.NotEmpty(t, hist, "the refused close must be recorded as a notice") | |
| 1042 | + | require.Equal(t, domain.NoticeSnapshotSaturated, hist[0].Code) | |
| 1043 | + | require.Equal(t, target.id, hist[0].SessionID) | |
| 1044 | + | ||
| 1045 | + | toasts := awaitToastCount(t, ac, 1) | |
| 1046 | + | require.Equal(t, domain.NoticeSnapshotSaturated, toasts[0].Code) | |
| 1047 | + | require.Equal(t, "couldn't close tab: session state not yet saved; try again", toasts[0].Message) | |
| 1048 | + | } | |
| 1049 | + | ||
| 975 | 1050 | func TestRenameTabDoesNotPersistForEphemeralSession(t *testing.T) { | |
| 976 | 1051 | sz := domain.Size{Cols: 80, Rows: 24} | |
| 977 | 1052 | p, release := newBlockingPTY(t) | |
| @@ -3,6 +3,7 @@ package daemon | |||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | "errors" | 5 | "errors" | ||
| 6 | + | "fmt" | |||
| 6 | "io" | 7 | "io" | ||
| 7 | "log/slog" | 8 | "log/slog" | ||
| 8 | "math" | 9 | "math" | ||
| @@ -20,6 +21,7 @@ import ( | |||||
| 20 | "github.com/bnema/vev/internal/ports" | 21 | "github.com/bnema/vev/internal/ports" | ||
| 21 | portsmocks "github.com/bnema/vev/internal/ports/mocks" | 22 | portsmocks "github.com/bnema/vev/internal/ports/mocks" | ||
| 22 | "github.com/bnema/vev/internal/usecase/picker" | 23 | "github.com/bnema/vev/internal/usecase/picker" | ||
| 24 | + | snapcodec "github.com/bnema/vev/internal/usecase/snapshot" | |||
| 23 | ) | 25 | ) | ||
| 24 | 26 | ||||
| 25 | // --- test doubles ----------------------------------------------------------- | 27 | // --- test doubles ----------------------------------------------------------- | ||
| @@ -972,6 +974,79 @@ func TestCloseActiveTabActivatesDestinationFloatingPane(t *testing.T) { | |||||
| 972 | requireFloatingInitialized(t, first) | 974 | requireFloatingInitialized(t, first) | ||
| 973 | } | 975 | } | ||
| 974 | 976 | ||||
| 977 | + | // TestCloseLastTabSaturatedSnapshotSurfacesNotice proves that when closing the | |||
| 978 | + | // last tab of a named session is refused because the snapshot worker cannot | |||
| 979 | + | // retain the final state, the session survives for a retry and the still | |||
| 980 | + | // attached client is told why, instead of the close silently no-oping. | |||
| 981 | + | // | |||
| 982 | + | // The snapshot worker is saturated deterministically with the same fixture as | |||
| 983 | + | // the kill-session ordering tests: one capture blocked in marshal, one in the | |||
| 984 | + | // bounded queue, and the terminal-retention fallback filled to capacity. No | |||
| 985 | + | // assertion runs on the worker goroutine; the marshal gate is released only on | |||
| 986 | + | // cleanup, after every assertion has run on the test goroutine. | |||
| 987 | + | func TestCloseLastTabSaturatedSnapshotSurfacesNotice(t *testing.T) { | |||
| 988 | + | store := discardSnapshotStore{} | |||
| 989 | + | d := newTestDaemon(t, portsmocks.NewMockPTYFactory(t), stubClock{}) | |||
| 990 | + | WithSnapshotStore(store)(d) | |||
| 991 | + | startSnapshotEncodeWorker(t, d) | |||
| 992 | + | ||||
| 993 | + | entered := make(chan struct{}) | |||
| 994 | + | release := make(chan struct{}) | |||
| 995 | + | // Release the sole blocked marshal on the test goroutine before the worker | |||
| 996 | + | // is stopped in cleanup, so teardown never waits on the gate. | |||
| 997 | + | defer close(release) | |||
| 998 | + | var calls atomic.Int32 | |||
| 999 | + | d.snapshotMarshal = func(s snapcodec.Session) ([]byte, error) { | |||
| 1000 | + | if calls.Add(1) == 1 { | |||
| 1001 | + | close(entered) | |||
| 1002 | + | <-release | |||
| 1003 | + | } | |||
| 1004 | + | return snapcodec.Marshal(s) | |||
| 1005 | + | } | |||
| 1006 | + | ||||
| 1007 | + | active := newSnapshotTestSession(t, "active", false, "/work") | |||
| 1008 | + | queued := newSnapshotTestSession(t, "queued", false, "/work") | |||
| 1009 | + | markSnapshotDirty(active) | |||
| 1010 | + | markSnapshotDirty(queued) | |||
| 1011 | + | require.True(t, d.scheduleSnapshot(active)) | |||
| 1012 | + | <-entered // the worker is now parked inside marshal for active | |||
| 1013 | + | require.True(t, d.scheduleSnapshot(queued), "one capture waits in the bounded queue") | |||
| 1014 | + | for i := range snapshotFinalQueueCapacity { | |||
| 1015 | + | s := newSnapshotTestSession(t, fmt.Sprintf("terminal-%d", i), false, "/work") | |||
| 1016 | + | markSnapshotDirty(s) | |||
| 1017 | + | require.True(t, d.scheduleFinalSnapshot(s), "fill the terminal-retention fallback to capacity") | |||
| 1018 | + | } | |||
| 1019 | + | ||||
| 1020 | + | // A single-tab named session whose client is still attached. | |||
| 1021 | + | target := newSnapshotTestSession(t, "target", false, "/work") | |||
| 1022 | + | tr, _ := newCapturingTransport(t) | |||
| 1023 | + | ac := &attachedClient{tr: tr, output: newOutputStateStream(), size: domain.Size{Cols: 80, Rows: 24}} | |||
| 1024 | + | ac.initOverlays() | |||
| 1025 | + | ac.setSession(target) | |||
| 1026 | + | target.client = ac | |||
| 1027 | + | d.mu.Lock() | |||
| 1028 | + | d.sessions[target.id] = target | |||
| 1029 | + | d.mu.Unlock() | |||
| 1030 | + | require.Len(t, target.tabs, 1) | |||
| 1031 | + | ||||
| 1032 | + | d.closeTab(target, target.tabs[0], false) | |||
| 1033 | + | ||||
| 1034 | + | // The refused close retains the session for a retry. | |||
| 1035 | + | d.mu.Lock() | |||
| 1036 | + | require.Same(t, target, d.sessions[target.id], "a refused close of the last tab must retain the session") | |||
| 1037 | + | d.mu.Unlock() | |||
| 1038 | + | ||||
| 1039 | + | // The refusal is recorded in history and shown on the still-attached client. | |||
| 1040 | + | hist := d.notices.history() | |||
| 1041 | + | require.NotEmpty(t, hist, "the refused close must be recorded as a notice") | |||
| 1042 | + | require.Equal(t, domain.NoticeSnapshotSaturated, hist[0].Code) | |||
| 1043 | + | require.Equal(t, target.id, hist[0].SessionID) | |||
| 1044 | + | ||||
| 1045 | + | toasts := awaitToastCount(t, ac, 1) | |||
| 1046 | + | require.Equal(t, domain.NoticeSnapshotSaturated, toasts[0].Code) | |||
| 1047 | + | require.Equal(t, "couldn't close tab: session state not yet saved; try again", toasts[0].Message) | |||
| 1048 | + | } | |||
| 1049 | + | ||||
| 975 | func TestRenameTabDoesNotPersistForEphemeralSession(t *testing.T) { | 1050 | func TestRenameTabDoesNotPersistForEphemeralSession(t *testing.T) { | ||
| 976 | sz := domain.Size{Cols: 80, Rows: 24} | 1051 | sz := domain.Size{Cols: 80, Rows: 24} | ||
| 977 | p, release := newBlockingPTY(t) | 1052 | p, release := newBlockingPTY(t) | ||