fix(daemon): surface snapshot write failures globally
When a periodic or terminal session snapshot fails to write to disk, the worker previously only logged the error; the user was never told their recent session state may be lost on restart. Raise a GLOBAL notice from the worker's write closure so every attached client learns of it: the failing session may already be torn down by the time the async write fails, so a session-scoped notice would be dead-on-arrival. The notify call is made with no lock held (clearSnapshotWorkerInFlight has released its own lock), and no teardown path holds d.mu while waiting on the worker, so routing through NotifyGlobal (which takes d.mu) cannot deadlock.
- commit
- c32a9ecc2345c2666c596cb83194ac10f604b9dd
- author
- brice · · 2026-07-20 19:04 UTC
- parent
- ac2dcc4
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
2 files · +50 -0
M internal/usecase/daemon/snapshot.go +5 -0
| @@ -579,6 +579,11 @@ func (d *Daemon) startSnapshotEncodeWorker() { | |||
| 579 | 579 | d.clearSnapshotWorkerInFlight(workerID, capture) | |
| 580 | 580 | if err != nil && workerCtx.Err() == nil { | |
| 581 | 581 | d.log.Warn("writing session snapshot failed", "err", err, "session", capture.name) | |
| 582 | + | // Global, not session-scoped: by now the session may already be | |
| 583 | + | // torn down, so a session notice would be dead-on-arrival. No lock | |
| 584 | + | // is held here (clearSnapshotWorkerInFlight released its own). | |
| 585 | + | d.NotifyGlobal(domain.NoticeError, domain.NoticeSnapshotWrite, | |
| 586 | + | "couldn't save session "+capture.name+"; recent state may be lost on restart", err) | |
| 582 | 587 | } | |
| 583 | 588 | d.finishSnapshotCapture(capture, err == nil && workerCtx.Err() == nil) | |
| 584 | 589 | return workerCtx.Err() == nil | |
| @@ -579,6 +579,11 @@ func (d *Daemon) startSnapshotEncodeWorker() { | |||||
| 579 | d.clearSnapshotWorkerInFlight(workerID, capture) | 579 | d.clearSnapshotWorkerInFlight(workerID, capture) | ||
| 580 | if err != nil && workerCtx.Err() == nil { | 580 | if err != nil && workerCtx.Err() == nil { | ||
| 581 | d.log.Warn("writing session snapshot failed", "err", err, "session", capture.name) | 581 | d.log.Warn("writing session snapshot failed", "err", err, "session", capture.name) | ||
| 582 | + | // Global, not session-scoped: by now the session may already be | |||
| 583 | + | // torn down, so a session notice would be dead-on-arrival. No lock | |||
| 584 | + | // is held here (clearSnapshotWorkerInFlight released its own). | |||
| 585 | + | d.NotifyGlobal(domain.NoticeError, domain.NoticeSnapshotWrite, | |||
| 586 | + | "couldn't save session "+capture.name+"; recent state may be lost on restart", err) | |||
| 582 | } | 587 | } | ||
| 583 | d.finishSnapshotCapture(capture, err == nil && workerCtx.Err() == nil) | 588 | d.finishSnapshotCapture(capture, err == nil && workerCtx.Err() == nil) | ||
| 584 | return workerCtx.Err() == nil | 589 | return workerCtx.Err() == nil | ||
A internal/usecase/daemon/snapshot_notice_test.go +45 -0
| @@ -0,0 +1,45 @@ | |||
| 1 | + | package daemon | |
| 2 | + | ||
| 3 | + | import ( | |
| 4 | + | "errors" | |
| 5 | + | "testing" | |
| 6 | + | ||
| 7 | + | "github.com/stretchr/testify/mock" | |
| 8 | + | "github.com/stretchr/testify/require" | |
| 9 | + | ||
| 10 | + | "github.com/bnema/vev/internal/domain" | |
| 11 | + | portsmocks "github.com/bnema/vev/internal/ports/mocks" | |
| 12 | + | ) | |
| 13 | + | ||
| 14 | + | // TestSnapshotWriteFailureNotifiesGlobally proves a failed snapshot write surfaces | |
| 15 | + | // as a GLOBAL notice: the failing session is never registered (it stands in for a | |
| 16 | + | // session already torn down by the time its async write fails), yet a client | |
| 17 | + | // attached to a DIFFERENT, surviving session still receives the toast. This is the | |
| 18 | + | // whole point of routing through NotifyGlobal instead of a session-scoped notice. | |
| 19 | + | func TestSnapshotWriteFailureNotifiesGlobally(t *testing.T) { | |
| 20 | + | // newNoticeFixture registers a survivor session "work"/"manual" owned by ac. | |
| 21 | + | d, _, survivorClient, _ := newNoticeFixture(t, newNoticeClock()) | |
| 22 | + | store := portsmocks.NewMockSnapshotStore(t) | |
| 23 | + | WithSnapshotStore(store)(d) | |
| 24 | + | d.procCwd = func(int) (string, error) { return "/pane", nil } | |
| 25 | + | startSnapshotEncodeWorker(t, d) | |
| 26 | + | ||
| 27 | + | // The failing session is deliberately NOT added to d.sessions. | |
| 28 | + | failing := newSnapshotTestSession(t, "failing", false, "/fallback") | |
| 29 | + | ||
| 30 | + | store.EXPECT().Write("failing", mock.Anything).RunAndReturn(func(string, []byte) error { | |
| 31 | + | return errors.New("disk full") | |
| 32 | + | }).Once() | |
| 33 | + | ||
| 34 | + | require.True(t, d.captureSession(failing)) | |
| 35 | + | ||
| 36 | + | // The write fails on the worker goroutine; awaitToastCount is the bounded, | |
| 37 | + | // poll-only completion signal. No require runs on the worker goroutine. | |
| 38 | + | toasts := awaitToastCount(t, survivorClient, 1) | |
| 39 | + | require.Equal(t, domain.NoticeSnapshotWrite, toasts[0].Code) | |
| 40 | + | require.Equal(t, "couldn't save session failing; recent state may be lost on restart", toasts[0].Message) | |
| 41 | + | require.Equal(t, domain.NoticeError, toasts[0].Severity) | |
| 42 | + | require.Equal(t, domain.SessionID(""), toasts[0].SessionID, | |
| 43 | + | "snapshot write failures are global, never scoped to the (possibly gone) failing session") | |
| 44 | + | require.Contains(t, toasts[0].Details, "disk full") | |
| 45 | + | } | |
| @@ -0,0 +1,45 @@ | |||||
| 1 | + | package daemon | |||
| 2 | + | ||||
| 3 | + | import ( | |||
| 4 | + | "errors" | |||
| 5 | + | "testing" | |||
| 6 | + | ||||
| 7 | + | "github.com/stretchr/testify/mock" | |||
| 8 | + | "github.com/stretchr/testify/require" | |||
| 9 | + | ||||
| 10 | + | "github.com/bnema/vev/internal/domain" | |||
| 11 | + | portsmocks "github.com/bnema/vev/internal/ports/mocks" | |||
| 12 | + | ) | |||
| 13 | + | ||||
| 14 | + | // TestSnapshotWriteFailureNotifiesGlobally proves a failed snapshot write surfaces | |||
| 15 | + | // as a GLOBAL notice: the failing session is never registered (it stands in for a | |||
| 16 | + | // session already torn down by the time its async write fails), yet a client | |||
| 17 | + | // attached to a DIFFERENT, surviving session still receives the toast. This is the | |||
| 18 | + | // whole point of routing through NotifyGlobal instead of a session-scoped notice. | |||
| 19 | + | func TestSnapshotWriteFailureNotifiesGlobally(t *testing.T) { | |||
| 20 | + | // newNoticeFixture registers a survivor session "work"/"manual" owned by ac. | |||
| 21 | + | d, _, survivorClient, _ := newNoticeFixture(t, newNoticeClock()) | |||
| 22 | + | store := portsmocks.NewMockSnapshotStore(t) | |||
| 23 | + | WithSnapshotStore(store)(d) | |||
| 24 | + | d.procCwd = func(int) (string, error) { return "/pane", nil } | |||
| 25 | + | startSnapshotEncodeWorker(t, d) | |||
| 26 | + | ||||
| 27 | + | // The failing session is deliberately NOT added to d.sessions. | |||
| 28 | + | failing := newSnapshotTestSession(t, "failing", false, "/fallback") | |||
| 29 | + | ||||
| 30 | + | store.EXPECT().Write("failing", mock.Anything).RunAndReturn(func(string, []byte) error { | |||
| 31 | + | return errors.New("disk full") | |||
| 32 | + | }).Once() | |||
| 33 | + | ||||
| 34 | + | require.True(t, d.captureSession(failing)) | |||
| 35 | + | ||||
| 36 | + | // The write fails on the worker goroutine; awaitToastCount is the bounded, | |||
| 37 | + | // poll-only completion signal. No require runs on the worker goroutine. | |||
| 38 | + | toasts := awaitToastCount(t, survivorClient, 1) | |||
| 39 | + | require.Equal(t, domain.NoticeSnapshotWrite, toasts[0].Code) | |||
| 40 | + | require.Equal(t, "couldn't save session failing; recent state may be lost on restart", toasts[0].Message) | |||
| 41 | + | require.Equal(t, domain.NoticeError, toasts[0].Severity) | |||
| 42 | + | require.Equal(t, domain.SessionID(""), toasts[0].SessionID, | |||
| 43 | + | "snapshot write failures are global, never scoped to the (possibly gone) failing session") | |||
| 44 | + | require.Contains(t, toasts[0].Details, "disk full") | |||
| 45 | + | } | |||