fix(daemon): address notification review findings
- commit
- 4fdfae1ee2eaf2ed7151439647245fd945d923f5
- author
- brice · · 2026-07-21 08:44 UTC
- parent
- 29b1a92
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
7 files · +167 -13
M internal/usecase/daemon/config.go +7 -2
| @@ -144,10 +144,15 @@ func (d *Daemon) buildCodeOverrides(configured map[string]string) (map[string]st | |||
| 144 | 144 | } | |
| 145 | 145 | ||
| 146 | 146 | dropped := false | |
| 147 | + | conflictCodes := make([]string, 0, len(claimants)) | |
| 147 | 148 | for code, slugs := range claimants { | |
| 148 | - | if len(slugs) < 2 { | |
| 149 | - | continue | |
| 149 | + | if len(slugs) > 1 { | |
| 150 | + | conflictCodes = append(conflictCodes, code) | |
| 150 | 151 | } | |
| 152 | + | } | |
| 153 | + | sort.Strings(conflictCodes) | |
| 154 | + | for _, code := range conflictCodes { | |
| 155 | + | slugs := claimants[code] | |
| 151 | 156 | sort.Strings(slugs) | |
| 152 | 157 | ||
| 153 | 158 | // If one claimant holds this code by default (no accepted | |
| @@ -144,10 +144,15 @@ func (d *Daemon) buildCodeOverrides(configured map[string]string) (map[string]st | |||||
| 144 | } | 144 | } | ||
| 145 | 145 | ||||
| 146 | dropped := false | 146 | dropped := false | ||
| 147 | + | conflictCodes := make([]string, 0, len(claimants)) | |||
| 147 | for code, slugs := range claimants { | 148 | for code, slugs := range claimants { | ||
| 148 | - | if len(slugs) < 2 { | 149 | + | if len(slugs) > 1 { |
| 149 | - | continue | 150 | + | conflictCodes = append(conflictCodes, code) |
| 150 | } | 151 | } | ||
| 152 | + | } | |||
| 153 | + | sort.Strings(conflictCodes) | |||
| 154 | + | for _, code := range conflictCodes { | |||
| 155 | + | slugs := claimants[code] | |||
| 151 | sort.Strings(slugs) | 156 | sort.Strings(slugs) | ||
| 152 | 157 | ||||
| 153 | // If one claimant holds this code by default (no accepted | 158 | // If one claimant holds this code by default (no accepted | ||
M internal/usecase/daemon/config_test.go +16 -0
| @@ -520,6 +520,22 @@ func TestCodeOverrideVacatedDefaultIsReusableRegardlessOfSlugOrder(t *testing.T) | |||
| 520 | 520 | require.Equal(t, "new-session", byCode["CNT"]) | |
| 521 | 521 | } | |
| 522 | 522 | ||
| 523 | + | func TestBuildCodeOverridesOrdersIndependentConflictWarnings(t *testing.T) { | |
| 524 | + | d := newTestDaemon(t, nil, stubClock{}) | |
| 525 | + | ||
| 526 | + | _, warnings := d.buildCodeOverrides(map[string]string{ | |
| 527 | + | "new-session": "ZZ", | |
| 528 | + | "new-tab": "ZZ", | |
| 529 | + | "split-left": "YY", | |
| 530 | + | "split-right": "YY", | |
| 531 | + | }) | |
| 532 | + | ||
| 533 | + | require.Equal(t, []domain.Warning{ | |
| 534 | + | {Msg: `command code "YY" for "split-right" conflicts with "split-left"`}, | |
| 535 | + | {Msg: `command code "ZZ" for "new-tab" conflicts with "new-session"`}, | |
| 536 | + | }, warnings) | |
| 537 | + | } | |
| 538 | + | ||
| 523 | 539 | func TestApplyConfigInvalidDefaultsWithoutPanic(t *testing.T) { | |
| 524 | 540 | d := newTestDaemon(t, nil, stubClock{}) | |
| 525 | 541 | require.NotPanics(t, func() { | |
| @@ -520,6 +520,22 @@ func TestCodeOverrideVacatedDefaultIsReusableRegardlessOfSlugOrder(t *testing.T) | |||||
| 520 | require.Equal(t, "new-session", byCode["CNT"]) | 520 | require.Equal(t, "new-session", byCode["CNT"]) | ||
| 521 | } | 521 | } | ||
| 522 | 522 | ||||
| 523 | + | func TestBuildCodeOverridesOrdersIndependentConflictWarnings(t *testing.T) { | |||
| 524 | + | d := newTestDaemon(t, nil, stubClock{}) | |||
| 525 | + | ||||
| 526 | + | _, warnings := d.buildCodeOverrides(map[string]string{ | |||
| 527 | + | "new-session": "ZZ", | |||
| 528 | + | "new-tab": "ZZ", | |||
| 529 | + | "split-left": "YY", | |||
| 530 | + | "split-right": "YY", | |||
| 531 | + | }) | |||
| 532 | + | ||||
| 533 | + | require.Equal(t, []domain.Warning{ | |||
| 534 | + | {Msg: `command code "YY" for "split-right" conflicts with "split-left"`}, | |||
| 535 | + | {Msg: `command code "ZZ" for "new-tab" conflicts with "new-session"`}, | |||
| 536 | + | }, warnings) | |||
| 537 | + | } | |||
| 538 | + | ||||
| 523 | func TestApplyConfigInvalidDefaultsWithoutPanic(t *testing.T) { | 539 | func TestApplyConfigInvalidDefaultsWithoutPanic(t *testing.T) { | ||
| 524 | d := newTestDaemon(t, nil, stubClock{}) | 540 | d := newTestDaemon(t, nil, stubClock{}) | ||
| 525 | require.NotPanics(t, func() { | 541 | require.NotPanics(t, func() { | ||
M internal/usecase/daemon/prompt.go +12 -0
| @@ -1,6 +1,7 @@ | |||
| 1 | 1 | package daemon | |
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | + | "errors" | |
| 4 | 5 | "strings" | |
| 5 | 6 | ||
| 6 | 7 | "github.com/bnema/vev/internal/domain" | |
| @@ -30,6 +31,14 @@ func (d *Daemon) closePrompt(ac *attachedClient) { | |||
| 30 | 31 | ac.overlays.promptMu.Unlock() | |
| 31 | 32 | } | |
| 32 | 33 | ||
| 34 | + | // promptValidationError identifies expected prompt feedback that should stay | |
| 35 | + | // inline rather than creating a history entry or toast. | |
| 36 | + | func promptValidationError(err error) bool { | |
| 37 | + | return errors.Is(err, domain.ErrInvalidSessionName) || | |
| 38 | + | errors.Is(err, errSessionNameRequired) || | |
| 39 | + | errors.Is(err, errSessionNameInUse) | |
| 40 | + | } | |
| 41 | + | ||
| 33 | 42 | func (d *Daemon) handlePromptInput(ac *attachedClient, data []byte) { | |
| 34 | 43 | sess := ac.currentSession() | |
| 35 | 44 | if sess == nil { | |
| @@ -76,6 +85,9 @@ func (d *Daemon) handlePromptInput(ac *attachedClient, data []byte) { | |||
| 76 | 85 | ac.overlays.prompt.SetError(err.Error()) | |
| 77 | 86 | } | |
| 78 | 87 | ac.overlays.promptMu.Unlock() | |
| 88 | + | if !promptValidationError(err) { | |
| 89 | + | d.reportError(sess, err) | |
| 90 | + | } | |
| 79 | 91 | d.invalidateRender(sess, ac, true, "prompt.go") | |
| 80 | 92 | return | |
| 81 | 93 | } | |
| @@ -1,6 +1,7 @@ | |||||
| 1 | package daemon | 1 | package daemon | ||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | + | "errors" | |||
| 4 | "strings" | 5 | "strings" | ||
| 5 | 6 | ||||
| 6 | "github.com/bnema/vev/internal/domain" | 7 | "github.com/bnema/vev/internal/domain" | ||
| @@ -30,6 +31,14 @@ func (d *Daemon) closePrompt(ac *attachedClient) { | |||||
| 30 | ac.overlays.promptMu.Unlock() | 31 | ac.overlays.promptMu.Unlock() | ||
| 31 | } | 32 | } | ||
| 32 | 33 | ||||
| 34 | + | // promptValidationError identifies expected prompt feedback that should stay | |||
| 35 | + | // inline rather than creating a history entry or toast. | |||
| 36 | + | func promptValidationError(err error) bool { | |||
| 37 | + | return errors.Is(err, domain.ErrInvalidSessionName) || | |||
| 38 | + | errors.Is(err, errSessionNameRequired) || | |||
| 39 | + | errors.Is(err, errSessionNameInUse) | |||
| 40 | + | } | |||
| 41 | + | ||||
| 33 | func (d *Daemon) handlePromptInput(ac *attachedClient, data []byte) { | 42 | func (d *Daemon) handlePromptInput(ac *attachedClient, data []byte) { | ||
| 34 | sess := ac.currentSession() | 43 | sess := ac.currentSession() | ||
| 35 | if sess == nil { | 44 | if sess == nil { | ||
| @@ -76,6 +85,9 @@ func (d *Daemon) handlePromptInput(ac *attachedClient, data []byte) { | |||||
| 76 | ac.overlays.prompt.SetError(err.Error()) | 85 | ac.overlays.prompt.SetError(err.Error()) | ||
| 77 | } | 86 | } | ||
| 78 | ac.overlays.promptMu.Unlock() | 87 | ac.overlays.promptMu.Unlock() | ||
| 88 | + | if !promptValidationError(err) { | |||
| 89 | + | d.reportError(sess, err) | |||
| 90 | + | } | |||
| 79 | d.invalidateRender(sess, ac, true, "prompt.go") | 91 | d.invalidateRender(sess, ac, true, "prompt.go") | ||
| 80 | return | 92 | return | ||
| 81 | } | 93 | } | ||
M internal/usecase/daemon/prompt_test.go +57 -1
| @@ -2,13 +2,21 @@ package daemon | |||
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | + | "errors" | |
| 5 | 6 | "testing" | |
| 6 | 7 | ||
| 8 | + | "github.com/stretchr/testify/require" | |
| 9 | + | ||
| 7 | 10 | "github.com/bnema/vev/internal/domain" | |
| 8 | 11 | "github.com/bnema/vev/internal/ports" | |
| 9 | - | "github.com/stretchr/testify/require" | |
| 10 | 12 | ) | |
| 11 | 13 | ||
| 14 | + | type failingPTYFactory struct{ err error } | |
| 15 | + | ||
| 16 | + | func (f failingPTYFactory) Open(context.Context, string, []string, []string, string, domain.Size) (ports.PTY, error) { | |
| 17 | + | return nil, f.err | |
| 18 | + | } | |
| 19 | + | ||
| 12 | 20 | func TestPromptModalGeometry(t *testing.T) { | |
| 13 | 21 | base := domain.Size{Cols: 100, Rows: 40} | |
| 14 | 22 | ||
| @@ -72,6 +80,54 @@ func TestPromptSubmitErrorKeepsPromptOpen(t *testing.T) { | |||
| 72 | 80 | require.Contains(t, string(out.Data), "name already in use") | |
| 73 | 81 | } | |
| 74 | 82 | ||
| 83 | + | func TestPromptSubmitSessionSpawnFailureReportsOneSafeNotice(t *testing.T) { | |
| 84 | + | p, release := newBlockingPTY(t) | |
| 85 | + | defer release() | |
| 86 | + | d, sess, ac, sends := newManualSessionWithPTYs(t, p) | |
| 87 | + | cause := errors.New("fork/exec /bin/sh: permission denied") | |
| 88 | + | d.ptys = failingPTYFactory{err: cause} | |
| 89 | + | ||
| 90 | + | // Submit through the actual prompt input boundary rather than calling its | |
| 91 | + | // callback directly. | |
| 92 | + | d.enterPrompt(sess, ac, " Create session ", "", func(name string) error { | |
| 93 | + | return d.createSessionAndSwitch(sess, ac, name) | |
| 94 | + | }) | |
| 95 | + | awaitFrame(t, sends, ports.MsgOutput) | |
| 96 | + | d.handlePromptInput(ac, []byte("new\r")) | |
| 97 | + | repaint := awaitFrame(t, sends, ports.MsgOutput) | |
| 98 | + | output, err := ports.UnmarshalOutput(repaint.Payload) | |
| 99 | + | require.NoError(t, err) | |
| 100 | + | require.Contains(t, string(output.Data), "couldn't create session: shell failed to start") | |
| 101 | + | ||
| 102 | + | history := d.notices.history() | |
| 103 | + | require.Len(t, history, 1, "the prompt boundary must report the failure once") | |
| 104 | + | notice := history[0] | |
| 105 | + | require.Equal(t, domain.NoticeSessionSpawn, notice.Code) | |
| 106 | + | require.Equal(t, "couldn't create session: shell failed to start", notice.Message) | |
| 107 | + | require.NotContains(t, notice.Message, cause.Error()) | |
| 108 | + | require.Contains(t, notice.Details, cause.Error()) | |
| 109 | + | require.True(t, ac.overlays.promptActive(), "failed submission stays inline") | |
| 110 | + | } | |
| 111 | + | ||
| 112 | + | func TestPromptSubmitValidationErrorStaysInlineOnly(t *testing.T) { | |
| 113 | + | p, release := newBlockingPTY(t) | |
| 114 | + | defer release() | |
| 115 | + | d, sess, ac, sends := newManualSessionWithPTYs(t, p) | |
| 116 | + | ||
| 117 | + | d.enterPrompt(sess, ac, " Create session ", "", func(name string) error { | |
| 118 | + | return d.createSessionAndSwitch(sess, ac, name) | |
| 119 | + | }) | |
| 120 | + | awaitFrame(t, sends, ports.MsgOutput) | |
| 121 | + | d.handlePromptInput(ac, []byte("invalid name\r")) | |
| 122 | + | repaint := awaitFrame(t, sends, ports.MsgOutput) | |
| 123 | + | output, err := ports.UnmarshalOutput(repaint.Payload) | |
| 124 | + | require.NoError(t, err) | |
| 125 | + | require.Contains(t, string(output.Data), domain.ErrInvalidSessionName.Error()) | |
| 126 | + | ||
| 127 | + | require.True(t, ac.overlays.promptActive()) | |
| 128 | + | require.Empty(t, d.notices.history()) | |
| 129 | + | } | |
| 130 | + | ||
| 75 | 131 | func TestPromptEscapeCancelsWithoutRename(t *testing.T) { | |
| 76 | 132 | p, release := newBlockingPTY(t) | |
| 77 | 133 | d, sess, ac, sends := newManualSessionWithPTYs(t, p) | |
| @@ -2,13 +2,21 @@ package daemon | |||||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | + | "errors" | |||
| 5 | "testing" | 6 | "testing" | ||
| 6 | 7 | ||||
| 8 | + | "github.com/stretchr/testify/require" | |||
| 9 | + | ||||
| 7 | "github.com/bnema/vev/internal/domain" | 10 | "github.com/bnema/vev/internal/domain" | ||
| 8 | "github.com/bnema/vev/internal/ports" | 11 | "github.com/bnema/vev/internal/ports" | ||
| 9 | - | "github.com/stretchr/testify/require" | |||
| 10 | ) | 12 | ) | ||
| 11 | 13 | ||||
| 14 | + | type failingPTYFactory struct{ err error } | |||
| 15 | + | ||||
| 16 | + | func (f failingPTYFactory) Open(context.Context, string, []string, []string, string, domain.Size) (ports.PTY, error) { | |||
| 17 | + | return nil, f.err | |||
| 18 | + | } | |||
| 19 | + | ||||
| 12 | func TestPromptModalGeometry(t *testing.T) { | 20 | func TestPromptModalGeometry(t *testing.T) { | ||
| 13 | base := domain.Size{Cols: 100, Rows: 40} | 21 | base := domain.Size{Cols: 100, Rows: 40} | ||
| 14 | 22 | ||||
| @@ -72,6 +80,54 @@ func TestPromptSubmitErrorKeepsPromptOpen(t *testing.T) { | |||||
| 72 | require.Contains(t, string(out.Data), "name already in use") | 80 | require.Contains(t, string(out.Data), "name already in use") | ||
| 73 | } | 81 | } | ||
| 74 | 82 | ||||
| 83 | + | func TestPromptSubmitSessionSpawnFailureReportsOneSafeNotice(t *testing.T) { | |||
| 84 | + | p, release := newBlockingPTY(t) | |||
| 85 | + | defer release() | |||
| 86 | + | d, sess, ac, sends := newManualSessionWithPTYs(t, p) | |||
| 87 | + | cause := errors.New("fork/exec /bin/sh: permission denied") | |||
| 88 | + | d.ptys = failingPTYFactory{err: cause} | |||
| 89 | + | ||||
| 90 | + | // Submit through the actual prompt input boundary rather than calling its | |||
| 91 | + | // callback directly. | |||
| 92 | + | d.enterPrompt(sess, ac, " Create session ", "", func(name string) error { | |||
| 93 | + | return d.createSessionAndSwitch(sess, ac, name) | |||
| 94 | + | }) | |||
| 95 | + | awaitFrame(t, sends, ports.MsgOutput) | |||
| 96 | + | d.handlePromptInput(ac, []byte("new\r")) | |||
| 97 | + | repaint := awaitFrame(t, sends, ports.MsgOutput) | |||
| 98 | + | output, err := ports.UnmarshalOutput(repaint.Payload) | |||
| 99 | + | require.NoError(t, err) | |||
| 100 | + | require.Contains(t, string(output.Data), "couldn't create session: shell failed to start") | |||
| 101 | + | ||||
| 102 | + | history := d.notices.history() | |||
| 103 | + | require.Len(t, history, 1, "the prompt boundary must report the failure once") | |||
| 104 | + | notice := history[0] | |||
| 105 | + | require.Equal(t, domain.NoticeSessionSpawn, notice.Code) | |||
| 106 | + | require.Equal(t, "couldn't create session: shell failed to start", notice.Message) | |||
| 107 | + | require.NotContains(t, notice.Message, cause.Error()) | |||
| 108 | + | require.Contains(t, notice.Details, cause.Error()) | |||
| 109 | + | require.True(t, ac.overlays.promptActive(), "failed submission stays inline") | |||
| 110 | + | } | |||
| 111 | + | ||||
| 112 | + | func TestPromptSubmitValidationErrorStaysInlineOnly(t *testing.T) { | |||
| 113 | + | p, release := newBlockingPTY(t) | |||
| 114 | + | defer release() | |||
| 115 | + | d, sess, ac, sends := newManualSessionWithPTYs(t, p) | |||
| 116 | + | ||||
| 117 | + | d.enterPrompt(sess, ac, " Create session ", "", func(name string) error { | |||
| 118 | + | return d.createSessionAndSwitch(sess, ac, name) | |||
| 119 | + | }) | |||
| 120 | + | awaitFrame(t, sends, ports.MsgOutput) | |||
| 121 | + | d.handlePromptInput(ac, []byte("invalid name\r")) | |||
| 122 | + | repaint := awaitFrame(t, sends, ports.MsgOutput) | |||
| 123 | + | output, err := ports.UnmarshalOutput(repaint.Payload) | |||
| 124 | + | require.NoError(t, err) | |||
| 125 | + | require.Contains(t, string(output.Data), domain.ErrInvalidSessionName.Error()) | |||
| 126 | + | ||||
| 127 | + | require.True(t, ac.overlays.promptActive()) | |||
| 128 | + | require.Empty(t, d.notices.history()) | |||
| 129 | + | } | |||
| 130 | + | ||||
| 75 | func TestPromptEscapeCancelsWithoutRename(t *testing.T) { | 131 | func TestPromptEscapeCancelsWithoutRename(t *testing.T) { | ||
| 76 | p, release := newBlockingPTY(t) | 132 | p, release := newBlockingPTY(t) | ||
| 77 | d, sess, ac, sends := newManualSessionWithPTYs(t, p) | 133 | d, sess, ac, sends := newManualSessionWithPTYs(t, p) | ||
M internal/usecase/daemon/render.go +4 -1
| @@ -101,7 +101,10 @@ func (d *Daemon) ptyReader(sess *session, tb *tab, p *pane) { | |||
| 101 | 101 | d.processPTYData(sess, tb, p, buf[:n], true) | |
| 102 | 102 | } | |
| 103 | 103 | if err != nil { | |
| 104 | - | d.log.Info("pane pty closed", "err", err, "session", sess.name) | |
| 104 | + | sess.mu.Lock() | |
| 105 | + | sessionName := sess.name | |
| 106 | + | sess.mu.Unlock() | |
| 107 | + | d.log.Info("pane pty closed", "err", err, "session", sessionName) | |
| 105 | 108 | if p.onExit != nil { | |
| 106 | 109 | p.onExit() | |
| 107 | 110 | } else { | |
| @@ -101,7 +101,10 @@ func (d *Daemon) ptyReader(sess *session, tb *tab, p *pane) { | |||||
| 101 | d.processPTYData(sess, tb, p, buf[:n], true) | 101 | d.processPTYData(sess, tb, p, buf[:n], true) | ||
| 102 | } | 102 | } | ||
| 103 | if err != nil { | 103 | if err != nil { | ||
| 104 | - | d.log.Info("pane pty closed", "err", err, "session", sess.name) | 104 | + | sess.mu.Lock() |
| 105 | + | sessionName := sess.name | |||
| 106 | + | sess.mu.Unlock() | |||
| 107 | + | d.log.Info("pane pty closed", "err", err, "session", sessionName) | |||
| 105 | if p.onExit != nil { | 108 | if p.onExit != nil { | ||
| 106 | p.onExit() | 109 | p.onExit() | ||
| 107 | } else { | 110 | } else { | ||
M internal/usecase/daemon/render_test.go +57 -0
| @@ -8,6 +8,7 @@ import ( | |||
| 8 | 8 | "log/slog" | |
| 9 | 9 | "strconv" | |
| 10 | 10 | "strings" | |
| 11 | + | "sync" | |
| 11 | 12 | "sync/atomic" | |
| 12 | 13 | "testing" | |
| 13 | 14 | "time" | |
| @@ -240,6 +241,62 @@ func TestPTYReaderLogsClosureWithoutNotification(t *testing.T) { | |||
| 240 | 241 | require.Empty(t, toasts, "PTY closure must not notify the user") | |
| 241 | 242 | } | |
| 242 | 243 | ||
| 244 | + | type concurrentExitPTY struct { | |
| 245 | + | started chan struct{} | |
| 246 | + | release chan struct{} | |
| 247 | + | } | |
| 248 | + | ||
| 249 | + | func newConcurrentExitPTY() *concurrentExitPTY { | |
| 250 | + | return &concurrentExitPTY{started: make(chan struct{}), release: make(chan struct{})} | |
| 251 | + | } | |
| 252 | + | ||
| 253 | + | func (p *concurrentExitPTY) Read([]byte) (int, error) { | |
| 254 | + | close(p.started) | |
| 255 | + | <-p.release | |
| 256 | + | return 0, io.EOF | |
| 257 | + | } | |
| 258 | + | func (*concurrentExitPTY) Write(b []byte) (int, error) { return len(b), nil } | |
| 259 | + | func (*concurrentExitPTY) Close() error { return nil } | |
| 260 | + | func (*concurrentExitPTY) Resize(domain.Size) error { return nil } | |
| 261 | + | func (*concurrentExitPTY) Pid() int { return 0 } | |
| 262 | + | func (*concurrentExitPTY) ForegroundPgid() (int, error) { | |
| 263 | + | return 0, nil | |
| 264 | + | } | |
| 265 | + | ||
| 266 | + | func TestPTYReaderReadsSessionNameUnderLockDuringConcurrentRename(t *testing.T) { | |
| 267 | + | // Repeated synchronized exits make the reader's closure log contend with a | |
| 268 | + | // rename. Running this under -race proves the name snapshot uses sess.mu. | |
| 269 | + | for range 100 { | |
| 270 | + | pty := newConcurrentExitPTY() | |
| 271 | + | d, sess, _, _ := newManualSessionWithPTYs(t, pty) | |
| 272 | + | pane := sess.tabs[0].focusedPane() | |
| 273 | + | pane.onExit = func() {} | |
| 274 | + | ||
| 275 | + | d.sessWg.Add(1) | |
| 276 | + | go d.ptyReader(sess, sess.tabs[0], pane) | |
| 277 | + | <-pty.started | |
| 278 | + | ||
| 279 | + | start := make(chan struct{}) | |
| 280 | + | renameErr := make(chan error, 1) | |
| 281 | + | var wg sync.WaitGroup | |
| 282 | + | wg.Add(2) | |
| 283 | + | go func() { | |
| 284 | + | defer wg.Done() | |
| 285 | + | <-start | |
| 286 | + | close(pty.release) | |
| 287 | + | }() | |
| 288 | + | go func() { | |
| 289 | + | defer wg.Done() | |
| 290 | + | <-start | |
| 291 | + | renameErr <- d.renameSession(sess, "renamed") | |
| 292 | + | }() | |
| 293 | + | close(start) | |
| 294 | + | wg.Wait() | |
| 295 | + | require.NoError(t, <-renameErr) | |
| 296 | + | d.sessWg.Wait() | |
| 297 | + | } | |
| 298 | + | } | |
| 299 | + | ||
| 243 | 300 | func TestPTYReaderSyncVisibilityTransitions(t *testing.T) { | |
| 244 | 301 | t.Run("inactive synchronized pane activates only after complete urgent frame", func(t *testing.T) { | |
| 245 | 302 | inactivePTY, inactiveSteps, inactiveProcessed := newChannelPTY(t) | |
| @@ -8,6 +8,7 @@ import ( | |||||
| 8 | "log/slog" | 8 | "log/slog" | ||
| 9 | "strconv" | 9 | "strconv" | ||
| 10 | "strings" | 10 | "strings" | ||
| 11 | + | "sync" | |||
| 11 | "sync/atomic" | 12 | "sync/atomic" | ||
| 12 | "testing" | 13 | "testing" | ||
| 13 | "time" | 14 | "time" | ||
| @@ -240,6 +241,62 @@ func TestPTYReaderLogsClosureWithoutNotification(t *testing.T) { | |||||
| 240 | require.Empty(t, toasts, "PTY closure must not notify the user") | 241 | require.Empty(t, toasts, "PTY closure must not notify the user") | ||
| 241 | } | 242 | } | ||
| 242 | 243 | ||||
| 244 | + | type concurrentExitPTY struct { | |||
| 245 | + | started chan struct{} | |||
| 246 | + | release chan struct{} | |||
| 247 | + | } | |||
| 248 | + | ||||
| 249 | + | func newConcurrentExitPTY() *concurrentExitPTY { | |||
| 250 | + | return &concurrentExitPTY{started: make(chan struct{}), release: make(chan struct{})} | |||
| 251 | + | } | |||
| 252 | + | ||||
| 253 | + | func (p *concurrentExitPTY) Read([]byte) (int, error) { | |||
| 254 | + | close(p.started) | |||
| 255 | + | <-p.release | |||
| 256 | + | return 0, io.EOF | |||
| 257 | + | } | |||
| 258 | + | func (*concurrentExitPTY) Write(b []byte) (int, error) { return len(b), nil } | |||
| 259 | + | func (*concurrentExitPTY) Close() error { return nil } | |||
| 260 | + | func (*concurrentExitPTY) Resize(domain.Size) error { return nil } | |||
| 261 | + | func (*concurrentExitPTY) Pid() int { return 0 } | |||
| 262 | + | func (*concurrentExitPTY) ForegroundPgid() (int, error) { | |||
| 263 | + | return 0, nil | |||
| 264 | + | } | |||
| 265 | + | ||||
| 266 | + | func TestPTYReaderReadsSessionNameUnderLockDuringConcurrentRename(t *testing.T) { | |||
| 267 | + | // Repeated synchronized exits make the reader's closure log contend with a | |||
| 268 | + | // rename. Running this under -race proves the name snapshot uses sess.mu. | |||
| 269 | + | for range 100 { | |||
| 270 | + | pty := newConcurrentExitPTY() | |||
| 271 | + | d, sess, _, _ := newManualSessionWithPTYs(t, pty) | |||
| 272 | + | pane := sess.tabs[0].focusedPane() | |||
| 273 | + | pane.onExit = func() {} | |||
| 274 | + | ||||
| 275 | + | d.sessWg.Add(1) | |||
| 276 | + | go d.ptyReader(sess, sess.tabs[0], pane) | |||
| 277 | + | <-pty.started | |||
| 278 | + | ||||
| 279 | + | start := make(chan struct{}) | |||
| 280 | + | renameErr := make(chan error, 1) | |||
| 281 | + | var wg sync.WaitGroup | |||
| 282 | + | wg.Add(2) | |||
| 283 | + | go func() { | |||
| 284 | + | defer wg.Done() | |||
| 285 | + | <-start | |||
| 286 | + | close(pty.release) | |||
| 287 | + | }() | |||
| 288 | + | go func() { | |||
| 289 | + | defer wg.Done() | |||
| 290 | + | <-start | |||
| 291 | + | renameErr <- d.renameSession(sess, "renamed") | |||
| 292 | + | }() | |||
| 293 | + | close(start) | |||
| 294 | + | wg.Wait() | |||
| 295 | + | require.NoError(t, <-renameErr) | |||
| 296 | + | d.sessWg.Wait() | |||
| 297 | + | } | |||
| 298 | + | } | |||
| 299 | + | ||||
| 243 | func TestPTYReaderSyncVisibilityTransitions(t *testing.T) { | 300 | func TestPTYReaderSyncVisibilityTransitions(t *testing.T) { | ||
| 244 | t.Run("inactive synchronized pane activates only after complete urgent frame", func(t *testing.T) { | 301 | t.Run("inactive synchronized pane activates only after complete urgent frame", func(t *testing.T) { | ||
| 245 | inactivePTY, inactiveSteps, inactiveProcessed := newChannelPTY(t) | 302 | inactivePTY, inactiveSteps, inactiveProcessed := newChannelPTY(t) | ||
M internal/usecase/daemon/session.go +14 -9
| @@ -19,6 +19,11 @@ import ( | |||
| 19 | 19 | "github.com/bnema/vev/internal/usecase/layout" | |
| 20 | 20 | ) | |
| 21 | 21 | ||
| 22 | + | var ( | |
| 23 | + | errSessionNameRequired = errors.New("name required") | |
| 24 | + | errSessionNameInUse = errors.New("name already in use") | |
| 25 | + | ) | |
| 26 | + | ||
| 22 | 27 | type terminalEnv struct { | |
| 23 | 28 | TrueColor bool | |
| 24 | 29 | } | |
| @@ -145,7 +150,7 @@ func (d *Daemon) createSessionLocked(name string, ephemeral bool, cwd string, sz | |||
| 145 | 150 | var err error | |
| 146 | 151 | createdAt, err = d.allocateLifecycleCreatedAtLocked() | |
| 147 | 152 | if err != nil { | |
| 148 | - | return nil, err | |
| 153 | + | return nil, domain.UserErr(domain.NoticeSessionSpawn, "couldn't create session", err) | |
| 149 | 154 | } | |
| 150 | 155 | } | |
| 151 | 156 | } | |
| @@ -160,14 +165,14 @@ func (d *Daemon) createSessionLocked(name string, ephemeral bool, cwd string, sz | |||
| 160 | 165 | tabStableID, paneStableID, err := d.newTabPaneStableIDs() | |
| 161 | 166 | if err != nil { | |
| 162 | 167 | closeTabs(tabs) | |
| 163 | - | return nil, err | |
| 168 | + | return nil, domain.UserErr(domain.NoticeSessionSpawn, "couldn't create session", err) | |
| 164 | 169 | } | |
| 165 | 170 | command, args := d.ptyCommand(env) | |
| 166 | 171 | pty, err := d.ptys.Open(d.serveCtx, command, args, childEnvFrom(env, name, tabStableID, paneStableID, term), cwd, tbSize) | |
| 167 | 172 | if err != nil { | |
| 168 | 173 | closeTabs(tabs) | |
| 169 | 174 | d.log.Warn("pty spawn failed", "err", err, "session", name, "kind", "session") | |
| 170 | - | return nil, fmt.Errorf("daemon: spawning session %q: %w", name, err) | |
| 175 | + | return nil, domain.UserErr(domain.NoticeSessionSpawn, "couldn't create session: shell failed to start", err) | |
| 171 | 176 | } | |
| 172 | 177 | tb := newTabWithStableID(tabStableID, paneStableID, pty, tbSize) | |
| 173 | 178 | if i < len(names) { | |
| @@ -207,7 +212,7 @@ func (d *Daemon) createSessionLocked(name string, ephemeral bool, cwd string, sz | |||
| 207 | 212 | if err := d.persist.Save(persist.Record{Name: name, Cwd: cwd, CreatedAt: createdAt, UpdatedAt: createdAt, LastUsedSeq: lastUsedSeq, TabNames: names}); err != nil { | |
| 208 | 213 | closeTabs(tabs) | |
| 209 | 214 | cancel() | |
| 210 | - | return nil, err | |
| 215 | + | return nil, domain.UserErr(domain.NoticeSessionSpawn, "couldn't create session", err) | |
| 211 | 216 | } | |
| 212 | 217 | delete(d.stopped, name) | |
| 213 | 218 | } | |
| @@ -233,7 +238,7 @@ func closeTabs(tabs []*tab) { | |||
| 233 | 238 | ||
| 234 | 239 | func (d *Daemon) createSessionAndSwitch(from *session, ac *attachedClient, name string) error { | |
| 235 | 240 | if name == "" { | |
| 236 | - | return errors.New("name required") | |
| 241 | + | return errSessionNameRequired | |
| 237 | 242 | } | |
| 238 | 243 | if err := domain.ValidateSessionName(name); err != nil { | |
| 239 | 244 | return err | |
| @@ -246,7 +251,7 @@ func (d *Daemon) createSessionAndSwitch(from *session, ac *attachedClient, name | |||
| 246 | 251 | } | |
| 247 | 252 | if d.nameLiveOrStoppedLocked(name) { | |
| 248 | 253 | d.mu.Unlock() | |
| 249 | - | return errors.New("name already in use") | |
| 254 | + | return errSessionNameInUse | |
| 250 | 255 | } | |
| 251 | 256 | from.mu.Lock() | |
| 252 | 257 | cwd := from.cwd | |
| @@ -494,7 +499,7 @@ func (s *session) switchRelative(delta int) bool { | |||
| 494 | 499 | ||
| 495 | 500 | func (d *Daemon) renameSession(sess *session, name string) error { | |
| 496 | 501 | if name == "" { | |
| 497 | - | return errors.New("name required") | |
| 502 | + | return errSessionNameRequired | |
| 498 | 503 | } | |
| 499 | 504 | if err := domain.ValidateSessionName(name); err != nil { | |
| 500 | 505 | return err | |
| @@ -502,10 +507,10 @@ func (d *Daemon) renameSession(sess *session, name string) error { | |||
| 502 | 507 | d.mu.Lock() | |
| 503 | 508 | defer d.mu.Unlock() | |
| 504 | 509 | if taken := d.findByNameLocked(name); taken != nil && taken != sess { | |
| 505 | - | return errors.New("name already in use") | |
| 510 | + | return errSessionNameInUse | |
| 506 | 511 | } | |
| 507 | 512 | if stopped, ok := d.stopped[name]; ok && stopped.name != sess.name { | |
| 508 | - | return errors.New("name already in use") | |
| 513 | + | return errSessionNameInUse | |
| 509 | 514 | } | |
| 510 | 515 | sess.mu.Lock() | |
| 511 | 516 | oldName := sess.name | |
| @@ -19,6 +19,11 @@ import ( | |||||
| 19 | "github.com/bnema/vev/internal/usecase/layout" | 19 | "github.com/bnema/vev/internal/usecase/layout" | ||
| 20 | ) | 20 | ) | ||
| 21 | 21 | ||||
| 22 | + | var ( | |||
| 23 | + | errSessionNameRequired = errors.New("name required") | |||
| 24 | + | errSessionNameInUse = errors.New("name already in use") | |||
| 25 | + | ) | |||
| 26 | + | ||||
| 22 | type terminalEnv struct { | 27 | type terminalEnv struct { | ||
| 23 | TrueColor bool | 28 | TrueColor bool | ||
| 24 | } | 29 | } | ||
| @@ -145,7 +150,7 @@ func (d *Daemon) createSessionLocked(name string, ephemeral bool, cwd string, sz | |||||
| 145 | var err error | 150 | var err error | ||
| 146 | createdAt, err = d.allocateLifecycleCreatedAtLocked() | 151 | createdAt, err = d.allocateLifecycleCreatedAtLocked() | ||
| 147 | if err != nil { | 152 | if err != nil { | ||
| 148 | - | return nil, err | 153 | + | return nil, domain.UserErr(domain.NoticeSessionSpawn, "couldn't create session", err) |
| 149 | } | 154 | } | ||
| 150 | } | 155 | } | ||
| 151 | } | 156 | } | ||
| @@ -160,14 +165,14 @@ func (d *Daemon) createSessionLocked(name string, ephemeral bool, cwd string, sz | |||||
| 160 | tabStableID, paneStableID, err := d.newTabPaneStableIDs() | 165 | tabStableID, paneStableID, err := d.newTabPaneStableIDs() | ||
| 161 | if err != nil { | 166 | if err != nil { | ||
| 162 | closeTabs(tabs) | 167 | closeTabs(tabs) | ||
| 163 | - | return nil, err | 168 | + | return nil, domain.UserErr(domain.NoticeSessionSpawn, "couldn't create session", err) |
| 164 | } | 169 | } | ||
| 165 | command, args := d.ptyCommand(env) | 170 | command, args := d.ptyCommand(env) | ||
| 166 | pty, err := d.ptys.Open(d.serveCtx, command, args, childEnvFrom(env, name, tabStableID, paneStableID, term), cwd, tbSize) | 171 | pty, err := d.ptys.Open(d.serveCtx, command, args, childEnvFrom(env, name, tabStableID, paneStableID, term), cwd, tbSize) | ||
| 167 | if err != nil { | 172 | if err != nil { | ||
| 168 | closeTabs(tabs) | 173 | closeTabs(tabs) | ||
| 169 | d.log.Warn("pty spawn failed", "err", err, "session", name, "kind", "session") | 174 | d.log.Warn("pty spawn failed", "err", err, "session", name, "kind", "session") | ||
| 170 | - | return nil, fmt.Errorf("daemon: spawning session %q: %w", name, err) | 175 | + | return nil, domain.UserErr(domain.NoticeSessionSpawn, "couldn't create session: shell failed to start", err) |
| 171 | } | 176 | } | ||
| 172 | tb := newTabWithStableID(tabStableID, paneStableID, pty, tbSize) | 177 | tb := newTabWithStableID(tabStableID, paneStableID, pty, tbSize) | ||
| 173 | if i < len(names) { | 178 | if i < len(names) { | ||
| @@ -207,7 +212,7 @@ func (d *Daemon) createSessionLocked(name string, ephemeral bool, cwd string, sz | |||||
| 207 | if err := d.persist.Save(persist.Record{Name: name, Cwd: cwd, CreatedAt: createdAt, UpdatedAt: createdAt, LastUsedSeq: lastUsedSeq, TabNames: names}); err != nil { | 212 | if err := d.persist.Save(persist.Record{Name: name, Cwd: cwd, CreatedAt: createdAt, UpdatedAt: createdAt, LastUsedSeq: lastUsedSeq, TabNames: names}); err != nil { | ||
| 208 | closeTabs(tabs) | 213 | closeTabs(tabs) | ||
| 209 | cancel() | 214 | cancel() | ||
| 210 | - | return nil, err | 215 | + | return nil, domain.UserErr(domain.NoticeSessionSpawn, "couldn't create session", err) |
| 211 | } | 216 | } | ||
| 212 | delete(d.stopped, name) | 217 | delete(d.stopped, name) | ||
| 213 | } | 218 | } | ||
| @@ -233,7 +238,7 @@ func closeTabs(tabs []*tab) { | |||||
| 233 | 238 | ||||
| 234 | func (d *Daemon) createSessionAndSwitch(from *session, ac *attachedClient, name string) error { | 239 | func (d *Daemon) createSessionAndSwitch(from *session, ac *attachedClient, name string) error { | ||
| 235 | if name == "" { | 240 | if name == "" { | ||
| 236 | - | return errors.New("name required") | 241 | + | return errSessionNameRequired |
| 237 | } | 242 | } | ||
| 238 | if err := domain.ValidateSessionName(name); err != nil { | 243 | if err := domain.ValidateSessionName(name); err != nil { | ||
| 239 | return err | 244 | return err | ||
| @@ -246,7 +251,7 @@ func (d *Daemon) createSessionAndSwitch(from *session, ac *attachedClient, name | |||||
| 246 | } | 251 | } | ||
| 247 | if d.nameLiveOrStoppedLocked(name) { | 252 | if d.nameLiveOrStoppedLocked(name) { | ||
| 248 | d.mu.Unlock() | 253 | d.mu.Unlock() | ||
| 249 | - | return errors.New("name already in use") | 254 | + | return errSessionNameInUse |
| 250 | } | 255 | } | ||
| 251 | from.mu.Lock() | 256 | from.mu.Lock() | ||
| 252 | cwd := from.cwd | 257 | cwd := from.cwd | ||
| @@ -494,7 +499,7 @@ func (s *session) switchRelative(delta int) bool { | |||||
| 494 | 499 | ||||
| 495 | func (d *Daemon) renameSession(sess *session, name string) error { | 500 | func (d *Daemon) renameSession(sess *session, name string) error { | ||
| 496 | if name == "" { | 501 | if name == "" { | ||
| 497 | - | return errors.New("name required") | 502 | + | return errSessionNameRequired |
| 498 | } | 503 | } | ||
| 499 | if err := domain.ValidateSessionName(name); err != nil { | 504 | if err := domain.ValidateSessionName(name); err != nil { | ||
| 500 | return err | 505 | return err | ||
| @@ -502,10 +507,10 @@ func (d *Daemon) renameSession(sess *session, name string) error { | |||||
| 502 | d.mu.Lock() | 507 | d.mu.Lock() | ||
| 503 | defer d.mu.Unlock() | 508 | defer d.mu.Unlock() | ||
| 504 | if taken := d.findByNameLocked(name); taken != nil && taken != sess { | 509 | if taken := d.findByNameLocked(name); taken != nil && taken != sess { | ||
| 505 | - | return errors.New("name already in use") | 510 | + | return errSessionNameInUse |
| 506 | } | 511 | } | ||
| 507 | if stopped, ok := d.stopped[name]; ok && stopped.name != sess.name { | 512 | if stopped, ok := d.stopped[name]; ok && stopped.name != sess.name { | ||
| 508 | - | return errors.New("name already in use") | 513 | + | return errSessionNameInUse |
| 509 | } | 514 | } | ||
| 510 | sess.mu.Lock() | 515 | sess.mu.Lock() | ||
| 511 | oldName := sess.name | 516 | oldName := sess.name | ||