fix(daemon): block unsafe picker recovery

commit: 27ab0c6c3a677eb4be9eb0b54fdf872fde1b7cab
author: brice · 2mo ago · 2026-07-25 23:48 UTC
parent: d19dd60
signature: GPG VerifiedCD88F9E564E0102646FB6077A1A07C7C7C3E349D · arch-gpg Verified for the committer email

3 files · +121 -1

M internal/usecase/daemon/daemon.go +10 -0


@@ -916,6 +916,13 @@ func (d *Daemon) waitForTargetRestore(name string) error {
916	916		return nil
917	917		}
918	918		switch stopped.state {
	919	+	case runtimeFresh:
	920	+	if stopped.record.Name == "" \|\| stopped.record.RecoveryState == domain.RecoveryFresh {
	921	+	d.mu.Unlock()
	922	+	return nil
	923	+	}
	924	+	d.mu.Unlock()
	925	+	return &protoErr{ports.ErrSessionDegraded, "session durable state is degraded: " + name}
919	926		case runtimeRestoring:
920	927		done := stopped.restoreDone
921	928		var shutdown <-chan struct{}

@@ -934,6 +941,9 @@ func (d *Daemon) waitForTargetRestore(name string) error {
934	941		return &protoErr{ports.ErrSessionDegraded, "session durable state is degraded: " + name}
935	942		default:
936	943		d.mu.Unlock()
	944	+	if stopped.record.Name != "" {
	945	+	return &protoErr{ports.ErrSessionDegraded, "session durable state is degraded: " + name}
	946	+	}
937	947		return nil
938	948		}
939	949		}


@@ -916,6 +916,13 @@ func (d *Daemon) waitForTargetRestore(name string) error {
916	return nil	916		return nil
917	}	917		}
918	switch stopped.state {	918		switch stopped.state {
		919	+	case runtimeFresh:
		920	+	if stopped.record.Name == "" \|\| stopped.record.RecoveryState == domain.RecoveryFresh {
		921	+	d.mu.Unlock()
		922	+	return nil
		923	+	}
		924	+	d.mu.Unlock()
		925	+	return &protoErr{ports.ErrSessionDegraded, "session durable state is degraded: " + name}
919	case runtimeRestoring:	926		case runtimeRestoring:
920	done := stopped.restoreDone	927		done := stopped.restoreDone
921	var shutdown <-chan struct{}	928		var shutdown <-chan struct{}

@@ -934,6 +941,9 @@ func (d *Daemon) waitForTargetRestore(name string) error {
934	return &protoErr{ports.ErrSessionDegraded, "session durable state is degraded: " + name}	941		return &protoErr{ports.ErrSessionDegraded, "session durable state is degraded: " + name}
935	default:	942		default:
936	d.mu.Unlock()	943		d.mu.Unlock()
		944	+	if stopped.record.Name != "" {
		945	+	return &protoErr{ports.ErrSessionDegraded, "session durable state is degraded: " + name}
		946	+	}
937	return nil	947		return nil
938	}	948		}
939	}	949		}

M internal/usecase/daemon/picker.go +11 -0


@@ -358,6 +358,13 @@ func (d Daemon) switchToTarget(from session, ac *attachedClient, target picker
358	358		// switchToTargetGuarded is the navigation-only variant whose guard is checked
359	359		// as part of the source ownership transfer.
360	360		func (d Daemon) switchToTargetGuarded(from session, ac *attachedClient, target picker.Target, guard sessionHandoffGuard) error {
	361	+	if target.Name != "" {
	362	+	if err := d.waitForTargetRestore(target.Name); err != nil {
	363	+	d.invalidateRender(from, ac, true, "picker.go")
	364	+	return domain.UserErr(domain.NoticeSessionUnavailable, "couldn't switch to that session", err)
	365	+	}
	366	+	}
	367	+
361	368		d.mu.Lock()
362	369		var (
363	370		targetSess *session

@@ -508,6 +515,10 @@ func (d Daemon) switchToActiveTargetLocked(from session, ac *attachedClient, t
508	515		// the handoff while d.mu is held. Creation failure leaves the source client
509	516		// and stopped record untouched.
510	517		func (d Daemon) resumeStoppedAndSwitchLocked(from session, ac attachedClient, target picker.Target, stopped stoppedSession) (session, []renderLifecycleCleanup, bool, error) {
	518	+	if stopped.record.Name != "" && (stopped.state != runtimeFresh \|\| stopped.record.RecoveryState != domain.RecoveryFresh) {
	519	+	return nil, nil, false, &protoErr{ports.ErrSessionDegraded, "session durable state is degraded: " + target.Name}
	520	+	}
	521	+
511	522		// The caller already holds d.mu. Keep handoff ownership atomic with global
512	523		// routing so it cannot select an attachment midway between sessions.
513	524		d.notices.routingMu.Lock()


@@ -358,6 +358,13 @@ func (d Daemon) switchToTarget(from session, ac *attachedClient, target picker
358	// switchToTargetGuarded is the navigation-only variant whose guard is checked	358		// switchToTargetGuarded is the navigation-only variant whose guard is checked
359	// as part of the source ownership transfer.	359		// as part of the source ownership transfer.
360	func (d Daemon) switchToTargetGuarded(from session, ac *attachedClient, target picker.Target, guard sessionHandoffGuard) error {	360		func (d Daemon) switchToTargetGuarded(from session, ac *attachedClient, target picker.Target, guard sessionHandoffGuard) error {
		361	+	if target.Name != "" {
		362	+	if err := d.waitForTargetRestore(target.Name); err != nil {
		363	+	d.invalidateRender(from, ac, true, "picker.go")
		364	+	return domain.UserErr(domain.NoticeSessionUnavailable, "couldn't switch to that session", err)
		365	+	}
		366	+	}
		367	+
361	d.mu.Lock()	368		d.mu.Lock()
362	var (	369		var (
363	targetSess *session	370		targetSess *session

@@ -508,6 +515,10 @@ func (d Daemon) switchToActiveTargetLocked(from session, ac *attachedClient, t
508	// the handoff while d.mu is held. Creation failure leaves the source client	515		// the handoff while d.mu is held. Creation failure leaves the source client
509	// and stopped record untouched.	516		// and stopped record untouched.
510	func (d Daemon) resumeStoppedAndSwitchLocked(from session, ac attachedClient, target picker.Target, stopped stoppedSession) (session, []renderLifecycleCleanup, bool, error) {	517		func (d Daemon) resumeStoppedAndSwitchLocked(from session, ac attachedClient, target picker.Target, stopped stoppedSession) (session, []renderLifecycleCleanup, bool, error) {
		518	+	if stopped.record.Name != "" && (stopped.state != runtimeFresh \|\| stopped.record.RecoveryState != domain.RecoveryFresh) {
		519	+	return nil, nil, false, &protoErr{ports.ErrSessionDegraded, "session durable state is degraded: " + target.Name}
		520	+	}
		521	+
511	// The caller already holds d.mu. Keep handoff ownership atomic with global	522		// The caller already holds d.mu. Keep handoff ownership atomic with global
512	// routing so it cannot select an attachment midway between sessions.	523		// routing so it cannot select an attachment midway between sessions.
513	d.notices.routingMu.Lock()	524		d.notices.routingMu.Lock()

M internal/usecase/daemon/picker_test.go +100 -1


@@ -11,6 +11,7 @@ import (
11	11		"github.com/stretchr/testify/require"
12	12
13	13		"github.com/bnema/vev/internal/domain"
	14	+	"github.com/bnema/vev/internal/persist"
14	15		"github.com/bnema/vev/internal/ports"
15	16		portsmocks "github.com/bnema/vev/internal/ports/mocks"
16	17		"github.com/bnema/vev/internal/usecase/keys"

@@ -242,6 +243,104 @@ func TestPickerViewsOmitsTerminalTitleWhenTabsConfigDisabled(t *testing.T) {
242	243		require.Equal(t, []picker.TabEntry{{Name: "1", Detail: " (vim)"}, {Name: "logs", Detail: " (sh)"}}, views[0].Tabs)
243	244		}
244	245
	246	+	func TestPickerWaitsForRestoringTargetBeforeSwitching(t *testing.T) {
	247	+	p, release := newBlockingPTY(t)
	248	+	defer release()
	249	+	d, from, ac, _ := newManualSessionWithPTYs(t, p)
	250	+	record := durableRecoveryRecord(0)
	251	+	record.Name = "restoring"
	252	+	state, done := initialRuntimeRecoveryState(record)
	253	+	d.stopped[record.Name] = stoppedSession{
	254	+	name: record.Name,
	255	+	createdAt: record.CreatedAt,
	256	+	incarnation: record.IncarnationID,
	257	+	record: record,
	258	+	state: state,
	259	+	restoreDone: done,
	260	+	}
	261	+	factory := &recoveryCountingPTYFactory{}
	262	+	d.ptys = factory
	263	+
	264	+	result := make(chan error, 1)
	265	+	go func() {
	266	+	result <- d.switchToTarget(from, ac, picker.Target{Name: record.Name, Stopped: true})
	267	+	}()
	268	+
	269	+	select {
	270	+	case err := <-result:
	271	+	t.Fatalf("picker switch returned before target restoration completed: %v", err)
	272	+	case <-time.After(25 * time.Millisecond):
	273	+	}
	274	+	require.Zero(t, factory.calls.Load())
	275	+	require.Same(t, from, ac.currentSession())
	276	+
	277	+	ctx, cancel := context.WithCancel(d.serveCtx)
	278	+	defer cancel()
	279	+	target := &session{id: "restored", name: record.Name, createdAt: record.CreatedAt, ctx: ctx, cancel: cancel, tabs: []*tab{newTab(nil, domain.Size{Cols: 80, Rows: 23})}}
	280	+	d.mu.Lock()
	281	+	delete(d.stopped, record.Name)
	282	+	d.sessions[target.id] = target
	283	+	d.mu.Unlock()
	284	+	closeRuntimeRestoreDone(done)
	285	+
	286	+	require.NoError(t, <-result)
	287	+	require.Same(t, target, ac.currentSession())
	288	+	}
	289	+
	290	+	func TestPickerRejectsCatalogueTargetsWithoutFreshRuntime(t *testing.T) {
	291	+	tests := []struct {
	292	+	name string
	293	+	recovery domain.RecoveryState
	294	+	runtimeState runtimeRecoveryState
	295	+	}{
	296	+	{name: "degraded", recovery: domain.RecoveryDegraded, runtimeState: runtimeDegraded},
	297	+	{name: "deleting", recovery: domain.RecoveryDeleting, runtimeState: runtimeDeleting},
	298	+	{name: "healthy with missing runtime state", recovery: domain.RecoveryHealthy},
	299	+	{name: "healthy without restored runtime", recovery: domain.RecoveryHealthy, runtimeState: runtimeHealthy},
	300	+	}
	301	+
	302	+	for _, tt := range tests {
	303	+	t.Run(tt.name, func(t *testing.T) {
	304	+	p, release := newBlockingPTY(t)
	305	+	defer release()
	306	+	d, from, ac, _ := newManualSessionWithPTYs(t, p)
	307	+	store, storeState := newMockStore(t)
	308	+	record := durableRecoveryRecord(0)
	309	+	record.Name = "unsafe"
	310	+	record.RecoveryState = tt.recovery
	311	+	catalogue := persist.New(store)
	312	+	WithCatalogue(catalogue, []domain.CatalogueRecord{record})(d)
	313	+	d.stopped[record.Name] = stoppedSession{
	314	+	name: record.Name,
	315	+	createdAt: record.CreatedAt,
	316	+	incarnation: record.IncarnationID,
	317	+	record: record,
	318	+	state: tt.runtimeState,
	319	+	}
	320	+	factory := &recoveryCountingPTYFactory{}
	321	+	d.ptys = factory
	322	+
	323	+	err := d.switchToTarget(from, ac, picker.Target{Name: record.Name, Stopped: true})
	324	+
	325	+	var userError *domain.UserError
	326	+	require.ErrorAs(t, err, &userError)
	327	+	require.Equal(t, domain.NoticeSessionUnavailable, userError.Code)
	328	+	var protocolError *protoErr
	329	+	require.ErrorAs(t, err, &protocolError)
	330	+	require.Equal(t, ports.ErrSessionDegraded, protocolError.code)
	331	+	require.Zero(t, factory.calls.Load(), "unsafe target must not open a PTY")
	332	+	require.Same(t, from, ac.currentSession())
	333	+
	334	+	storeState.mu.Lock()
	335	+	sets := storeState.sets
	336	+	storeState.mu.Unlock()
	337	+	require.Zero(t, sets, "unsafe target must not mutate the catalogue")
	338	+	require.Equal(t, record, d.stopped[record.Name].record)
	339	+	require.Equal(t, tt.runtimeState, d.stopped[record.Name].state)
	340	+	})
	341	+	}
	342	+	}
	343	+
245	344		func TestPickerResumesStoppedSessionWithPersistedTabNames(t *testing.T) {
246	345		p1, release1 := newBlockingPTY(t)
247	346		p2, release2 := newBlockingPTY(t)

@@ -251,7 +350,7 @@ func TestPickerResumesStoppedSessionWithPersistedTabNames(t *testing.T) {
251	350		defer release3()
252	351		d, from, ac, sends := newManualSessionWithPTYs(t, p1)
253	352		d.ptys = newFactorySeq(t, p2, p3)
254		-	d.stopped["work"] = stoppedSession{name: "work", cwd: "/tmp/work", createdAt: 7, tabNames: []string{"shell", "logs"}}
	353	+	d.stopped["work"] = stoppedSession{name: "work", cwd: "/tmp/work", createdAt: 7, tabNames: []string{"shell", "logs"}, record: domain.CatalogueRecord{Name: "work", RecoveryState: domain.RecoveryFresh}, state: runtimeFresh}
255	354
256	355		d.resumeStoppedAndSwitch(from, ac, picker.Target{Name: "work", Stopped: true})
257	356		awaitFrame(t, sends, ports.MsgOutput)


@@ -11,6 +11,7 @@ import (
11		"github.com/stretchr/testify/require"	11		"github.com/stretchr/testify/require"
12			12
13		"github.com/bnema/vev/internal/domain"	13		"github.com/bnema/vev/internal/domain"
			14	+	"github.com/bnema/vev/internal/persist"
14		"github.com/bnema/vev/internal/ports"	15		"github.com/bnema/vev/internal/ports"
15		portsmocks "github.com/bnema/vev/internal/ports/mocks"	16		portsmocks "github.com/bnema/vev/internal/ports/mocks"
16		"github.com/bnema/vev/internal/usecase/keys"	17		"github.com/bnema/vev/internal/usecase/keys"

@@ -242,6 +243,104 @@ func TestPickerViewsOmitsTerminalTitleWhenTabsConfigDisabled(t *testing.T) {
242		require.Equal(t, []picker.TabEntry{{Name: "1", Detail: " (vim)"}, {Name: "logs", Detail: " (sh)"}}, views[0].Tabs)	243		require.Equal(t, []picker.TabEntry{{Name: "1", Detail: " (vim)"}, {Name: "logs", Detail: " (sh)"}}, views[0].Tabs)
243		}	244		}
244			245
			246	+	func TestPickerWaitsForRestoringTargetBeforeSwitching(t *testing.T) {
			247	+	p, release := newBlockingPTY(t)
			248	+	defer release()
			249	+	d, from, ac, _ := newManualSessionWithPTYs(t, p)
			250	+	record := durableRecoveryRecord(0)
			251	+	record.Name = "restoring"
			252	+	state, done := initialRuntimeRecoveryState(record)
			253	+	d.stopped[record.Name] = stoppedSession{
			254	+	name: record.Name,
			255	+	createdAt: record.CreatedAt,
			256	+	incarnation: record.IncarnationID,
			257	+	record: record,
			258	+	state: state,
			259	+	restoreDone: done,
			260	+	}
			261	+	factory := &recoveryCountingPTYFactory{}
			262	+	d.ptys = factory
			263	+
			264	+	result := make(chan error, 1)
			265	+	go func() {
			266	+	result <- d.switchToTarget(from, ac, picker.Target{Name: record.Name, Stopped: true})
			267	+	}()
			268	+
			269	+	select {
			270	+	case err := <-result:
			271	+	t.Fatalf("picker switch returned before target restoration completed: %v", err)
			272	+	case <-time.After(25 * time.Millisecond):
			273	+	}
			274	+	require.Zero(t, factory.calls.Load())
			275	+	require.Same(t, from, ac.currentSession())
			276	+
			277	+	ctx, cancel := context.WithCancel(d.serveCtx)
			278	+	defer cancel()
			279	+	target := &session{id: "restored", name: record.Name, createdAt: record.CreatedAt, ctx: ctx, cancel: cancel, tabs: []*tab{newTab(nil, domain.Size{Cols: 80, Rows: 23})}}
			280	+	d.mu.Lock()
			281	+	delete(d.stopped, record.Name)
			282	+	d.sessions[target.id] = target
			283	+	d.mu.Unlock()
			284	+	closeRuntimeRestoreDone(done)
			285	+
			286	+	require.NoError(t, <-result)
			287	+	require.Same(t, target, ac.currentSession())
			288	+	}
			289	+
			290	+	func TestPickerRejectsCatalogueTargetsWithoutFreshRuntime(t *testing.T) {
			291	+	tests := []struct {
			292	+	name string
			293	+	recovery domain.RecoveryState
			294	+	runtimeState runtimeRecoveryState
			295	+	}{
			296	+	{name: "degraded", recovery: domain.RecoveryDegraded, runtimeState: runtimeDegraded},
			297	+	{name: "deleting", recovery: domain.RecoveryDeleting, runtimeState: runtimeDeleting},
			298	+	{name: "healthy with missing runtime state", recovery: domain.RecoveryHealthy},
			299	+	{name: "healthy without restored runtime", recovery: domain.RecoveryHealthy, runtimeState: runtimeHealthy},
			300	+	}
			301	+
			302	+	for _, tt := range tests {
			303	+	t.Run(tt.name, func(t *testing.T) {
			304	+	p, release := newBlockingPTY(t)
			305	+	defer release()
			306	+	d, from, ac, _ := newManualSessionWithPTYs(t, p)
			307	+	store, storeState := newMockStore(t)
			308	+	record := durableRecoveryRecord(0)
			309	+	record.Name = "unsafe"
			310	+	record.RecoveryState = tt.recovery
			311	+	catalogue := persist.New(store)
			312	+	WithCatalogue(catalogue, []domain.CatalogueRecord{record})(d)
			313	+	d.stopped[record.Name] = stoppedSession{
			314	+	name: record.Name,
			315	+	createdAt: record.CreatedAt,
			316	+	incarnation: record.IncarnationID,
			317	+	record: record,
			318	+	state: tt.runtimeState,
			319	+	}
			320	+	factory := &recoveryCountingPTYFactory{}
			321	+	d.ptys = factory
			322	+
			323	+	err := d.switchToTarget(from, ac, picker.Target{Name: record.Name, Stopped: true})
			324	+
			325	+	var userError *domain.UserError
			326	+	require.ErrorAs(t, err, &userError)
			327	+	require.Equal(t, domain.NoticeSessionUnavailable, userError.Code)
			328	+	var protocolError *protoErr
			329	+	require.ErrorAs(t, err, &protocolError)
			330	+	require.Equal(t, ports.ErrSessionDegraded, protocolError.code)
			331	+	require.Zero(t, factory.calls.Load(), "unsafe target must not open a PTY")
			332	+	require.Same(t, from, ac.currentSession())
			333	+
			334	+	storeState.mu.Lock()
			335	+	sets := storeState.sets
			336	+	storeState.mu.Unlock()
			337	+	require.Zero(t, sets, "unsafe target must not mutate the catalogue")
			338	+	require.Equal(t, record, d.stopped[record.Name].record)
			339	+	require.Equal(t, tt.runtimeState, d.stopped[record.Name].state)
			340	+	})
			341	+	}
			342	+	}
			343	+
245		func TestPickerResumesStoppedSessionWithPersistedTabNames(t *testing.T) {	344		func TestPickerResumesStoppedSessionWithPersistedTabNames(t *testing.T) {
246		p1, release1 := newBlockingPTY(t)	345		p1, release1 := newBlockingPTY(t)
247		p2, release2 := newBlockingPTY(t)	346		p2, release2 := newBlockingPTY(t)

@@ -251,7 +350,7 @@ func TestPickerResumesStoppedSessionWithPersistedTabNames(t *testing.T) {
251		defer release3()	350		defer release3()
252		d, from, ac, sends := newManualSessionWithPTYs(t, p1)	351		d, from, ac, sends := newManualSessionWithPTYs(t, p1)
253		d.ptys = newFactorySeq(t, p2, p3)	352		d.ptys = newFactorySeq(t, p2, p3)
254	-	d.stopped["work"] = stoppedSession{name: "work", cwd: "/tmp/work", createdAt: 7, tabNames: []string{"shell", "logs"}}	353	+	d.stopped["work"] = stoppedSession{name: "work", cwd: "/tmp/work", createdAt: 7, tabNames: []string{"shell", "logs"}, record: domain.CatalogueRecord{Name: "work", RecoveryState: domain.RecoveryFresh}, state: runtimeFresh}
255			354
256		d.resumeStoppedAndSwitch(from, ac, picker.Target{Name: "work", Stopped: true})	355		d.resumeStoppedAndSwitch(from, ac, picker.Target{Name: "work", Stopped: true})
257		awaitFrame(t, sends, ports.MsgOutput)	356		awaitFrame(t, sends, ports.MsgOutput)