fix: address coderabbit durability findings
- commit
- 90fb24b685e40f5c4c47b63f3b44b80711c24d04
- author
- brice · · 2026-07-26 14:20 UTC
- parent
- 0ffe39d
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
20 files · +178 -55
M docs/durable-session-recovery.md +1 -4
| @@ -8,12 +8,9 @@ vev opens named-session state only while holding `$XDG_RUNTIME_DIR/vev/lifecycle | |||
| 8 | 8 | ||
| 9 | 9 | ## Recovery commands | |
| 10 | 10 | ||
| 11 | - | - `vev cmd -s NAME session-recovery retry` | |
| 12 | - | - `vev cmd -s NAME session-recovery restore GENERATION` | |
| 13 | - | - `vev cmd -s NAME session-recovery export /ABSOLUTE/PATH` | |
| 14 | 11 | - `vev cmd -s NAME session-recovery discard` | |
| 15 | 12 | ||
| 16 | - | Retry validates persisted state without replacing it. Restore validates and promotes the selected catalogue fallback. Export writes recoverable data without mutation and requires an absolute destination path. Discard creates a new incarnation and retains the old record and snapshots under `quarantine/` until an explicit later purge. | |
| 13 | + | Discard creates a new incarnation and retains the old record and snapshots under `snapshots/quarantine/` until an explicit later purge. | |
| 17 | 14 | ||
| 18 | 15 | ## Migration and retention | |
| 19 | 16 | ||
| @@ -8,12 +8,9 @@ vev opens named-session state only while holding `$XDG_RUNTIME_DIR/vev/lifecycle | |||||
| 8 | 8 | ||||
| 9 | ## Recovery commands | 9 | ## Recovery commands | ||
| 10 | 10 | ||||
| 11 | - | - `vev cmd -s NAME session-recovery retry` | |||
| 12 | - | - `vev cmd -s NAME session-recovery restore GENERATION` | |||
| 13 | - | - `vev cmd -s NAME session-recovery export /ABSOLUTE/PATH` | |||
| 14 | - `vev cmd -s NAME session-recovery discard` | 11 | - `vev cmd -s NAME session-recovery discard` | ||
| 15 | 12 | ||||
| 16 | - | Retry validates persisted state without replacing it. Restore validates and promotes the selected catalogue fallback. Export writes recoverable data without mutation and requires an absolute destination path. Discard creates a new incarnation and retains the old record and snapshots under `quarantine/` until an explicit later purge. | 13 | + | Discard creates a new incarnation and retains the old record and snapshots under `snapshots/quarantine/` until an explicit later purge. |
| 17 | 14 | ||||
| 18 | ## Migration and retention | 15 | ## Migration and retention | ||
| 19 | 16 | ||||
M internal/adapters/lifecycle/flock_test.go +2 -0
| @@ -1,3 +1,5 @@ | |||
| 1 | + | //go:build unix | |
| 2 | + | ||
| 1 | 3 | package lifecycle | |
| 2 | 4 | ||
| 3 | 5 | import ( | |
| @@ -1,3 +1,5 @@ | |||||
| 1 | + | //go:build unix | |||
| 2 | + | ||||
| 1 | package lifecycle | 3 | package lifecycle | ||
| 2 | 4 | ||||
| 3 | import ( | 5 | import ( | ||
M internal/adapters/snapshot/maintenance_delete.go +15 -1
| @@ -24,7 +24,7 @@ func (r *Repository) DeleteIncarnation(ctx context.Context, id domain.Incarnatio | |||
| 24 | 24 | defer r.unlockSession(lock) | |
| 25 | 25 | ||
| 26 | 26 | path := r.sessionPath(id) | |
| 27 | - | if err := os.RemoveAll(path); err != nil { | |
| 27 | + | if err := r.removeIncarnationLocked(id); err != nil { | |
| 28 | 28 | return fmt.Errorf("remove snapshot incarnation %s: %w", id.String(), safeFilesystemError(err)) | |
| 29 | 29 | } | |
| 30 | 30 | if err := ctx.Err(); err != nil { | |
| @@ -38,3 +38,17 @@ func (r *Repository) DeleteIncarnation(ctx context.Context, id domain.Incarnatio | |||
| 38 | 38 | } | |
| 39 | 39 | return r.syncDirectory(parent) | |
| 40 | 40 | } | |
| 41 | + | ||
| 42 | + | // removeIncarnationLocked removes an incarnation through a root-pinned path. | |
| 43 | + | // The caller must hold the incarnation's session lock. | |
| 44 | + | func (r *Repository) removeIncarnationLocked(id domain.IncarnationID) (err error) { | |
| 45 | + | root, err := r.openRoot() | |
| 46 | + | if errors.Is(err, os.ErrNotExist) { | |
| 47 | + | return nil | |
| 48 | + | } | |
| 49 | + | if err != nil { | |
| 50 | + | return err | |
| 51 | + | } | |
| 52 | + | defer func() { joinCloseError(&err, "close snapshot root", r.closeRoot(root)) }() | |
| 53 | + | return root.RemoveAll(filepath.Join(repositorySessionsDir, id.String())) | |
| 54 | + | } | |
| @@ -24,7 +24,7 @@ func (r *Repository) DeleteIncarnation(ctx context.Context, id domain.Incarnatio | |||||
| 24 | defer r.unlockSession(lock) | 24 | defer r.unlockSession(lock) | ||
| 25 | 25 | ||||
| 26 | path := r.sessionPath(id) | 26 | path := r.sessionPath(id) | ||
| 27 | - | if err := os.RemoveAll(path); err != nil { | 27 | + | if err := r.removeIncarnationLocked(id); err != nil { |
| 28 | return fmt.Errorf("remove snapshot incarnation %s: %w", id.String(), safeFilesystemError(err)) | 28 | return fmt.Errorf("remove snapshot incarnation %s: %w", id.String(), safeFilesystemError(err)) | ||
| 29 | } | 29 | } | ||
| 30 | if err := ctx.Err(); err != nil { | 30 | if err := ctx.Err(); err != nil { | ||
| @@ -38,3 +38,17 @@ func (r *Repository) DeleteIncarnation(ctx context.Context, id domain.Incarnatio | |||||
| 38 | } | 38 | } | ||
| 39 | return r.syncDirectory(parent) | 39 | return r.syncDirectory(parent) | ||
| 40 | } | 40 | } | ||
| 41 | + | ||||
| 42 | + | // removeIncarnationLocked removes an incarnation through a root-pinned path. | |||
| 43 | + | // The caller must hold the incarnation's session lock. | |||
| 44 | + | func (r *Repository) removeIncarnationLocked(id domain.IncarnationID) (err error) { | |||
| 45 | + | root, err := r.openRoot() | |||
| 46 | + | if errors.Is(err, os.ErrNotExist) { | |||
| 47 | + | return nil | |||
| 48 | + | } | |||
| 49 | + | if err != nil { | |||
| 50 | + | return err | |||
| 51 | + | } | |||
| 52 | + | defer func() { joinCloseError(&err, "close snapshot root", r.closeRoot(root)) }() | |||
| 53 | + | return root.RemoveAll(filepath.Join(repositorySessionsDir, id.String())) | |||
| 54 | + | } | |||
M internal/adapters/snapshot/maintenance_gc.go +15 -3
| @@ -43,12 +43,24 @@ func (r *Repository) CollectGarbage(ctx context.Context, keep map[domain.Incarna | |||
| 43 | 43 | } | |
| 44 | 44 | committed, known := keep[id] | |
| 45 | 45 | if !known { | |
| 46 | - | path := r.sessionPath(id) | |
| 47 | - | if err := os.RemoveAll(path); err != nil { | |
| 46 | + | key, keyErr := incarnationKey(id) | |
| 47 | + | if keyErr != nil { | |
| 48 | + | collected = errors.Join(collected, keyErr) | |
| 49 | + | continue | |
| 50 | + | } | |
| 51 | + | lock := r.lockSession(key) | |
| 52 | + | _, known = keep[id] | |
| 53 | + | if !known { | |
| 54 | + | err = r.removeIncarnationLocked(id) | |
| 55 | + | } | |
| 56 | + | r.unlockSession(lock) | |
| 57 | + | if err != nil { | |
| 48 | 58 | collected = errors.Join(collected, fmt.Errorf("remove orphan snapshot incarnation %s: %w", id.String(), safeFilesystemError(err))) | |
| 49 | 59 | continue | |
| 50 | 60 | } | |
| 51 | - | r.log.Info("snapshot_garbage_collected", "incarnation", id.String(), "action", "remove-incarnation") | |
| 61 | + | if !known { | |
| 62 | + | r.log.Info("snapshot_garbage_collected", "incarnation", id.String(), "action", "remove-incarnation") | |
| 63 | + | } | |
| 52 | 64 | continue | |
| 53 | 65 | } | |
| 54 | 66 | if err := r.pruneGenerations(ctx, id, committed); err != nil { | |
| @@ -43,12 +43,24 @@ func (r *Repository) CollectGarbage(ctx context.Context, keep map[domain.Incarna | |||||
| 43 | } | 43 | } | ||
| 44 | committed, known := keep[id] | 44 | committed, known := keep[id] | ||
| 45 | if !known { | 45 | if !known { | ||
| 46 | - | path := r.sessionPath(id) | 46 | + | key, keyErr := incarnationKey(id) |
| 47 | - | if err := os.RemoveAll(path); err != nil { | 47 | + | if keyErr != nil { |
| 48 | + | collected = errors.Join(collected, keyErr) | |||
| 49 | + | continue | |||
| 50 | + | } | |||
| 51 | + | lock := r.lockSession(key) | |||
| 52 | + | _, known = keep[id] | |||
| 53 | + | if !known { | |||
| 54 | + | err = r.removeIncarnationLocked(id) | |||
| 55 | + | } | |||
| 56 | + | r.unlockSession(lock) | |||
| 57 | + | if err != nil { | |||
| 48 | collected = errors.Join(collected, fmt.Errorf("remove orphan snapshot incarnation %s: %w", id.String(), safeFilesystemError(err))) | 58 | collected = errors.Join(collected, fmt.Errorf("remove orphan snapshot incarnation %s: %w", id.String(), safeFilesystemError(err))) | ||
| 49 | continue | 59 | continue | ||
| 50 | } | 60 | } | ||
| 51 | - | r.log.Info("snapshot_garbage_collected", "incarnation", id.String(), "action", "remove-incarnation") | 61 | + | if !known { |
| 62 | + | r.log.Info("snapshot_garbage_collected", "incarnation", id.String(), "action", "remove-incarnation") | |||
| 63 | + | } | |||
| 52 | continue | 64 | continue | ||
| 53 | } | 65 | } | ||
| 54 | if err := r.pruneGenerations(ctx, id, committed); err != nil { | 66 | if err := r.pruneGenerations(ctx, id, committed); err != nil { | ||
M internal/adapters/snapshot/publish.go +1 -0
| @@ -144,6 +144,7 @@ func (r *Repository) Publish(ctx context.Context, publication ports.SnapshotPubl | |||
| 144 | 144 | // because its manifest bytes happen to match the requested publication. | |
| 145 | 145 | func (r *Repository) unchangedPublication(ctx context.Context, publication ports.SnapshotPublication) (bool, error) { | |
| 146 | 146 | generation, digest, err := r.readHead(publication.IncarnationID) | |
| 147 | + | _ = err // A missing or unreadable HEAD is not treated as a replay. | |
| 147 | 148 | if err != nil || generation != publication.Generation { | |
| 148 | 149 | return false, nil | |
| 149 | 150 | } | |
| @@ -144,6 +144,7 @@ func (r *Repository) Publish(ctx context.Context, publication ports.SnapshotPubl | |||||
| 144 | // because its manifest bytes happen to match the requested publication. | 144 | // because its manifest bytes happen to match the requested publication. | ||
| 145 | func (r *Repository) unchangedPublication(ctx context.Context, publication ports.SnapshotPublication) (bool, error) { | 145 | func (r *Repository) unchangedPublication(ctx context.Context, publication ports.SnapshotPublication) (bool, error) { | ||
| 146 | generation, digest, err := r.readHead(publication.IncarnationID) | 146 | generation, digest, err := r.readHead(publication.IncarnationID) | ||
| 147 | + | _ = err // A missing or unreadable HEAD is not treated as a replay. | |||
| 147 | if err != nil || generation != publication.Generation { | 148 | if err != nil || generation != publication.Generation { | ||
| 148 | return false, nil | 149 | return false, nil | ||
| 149 | } | 150 | } | ||
M internal/adapters/snapshot/repository.go +10 -6
| @@ -30,12 +30,16 @@ type Repository struct { | |||
| 30 | 30 | ||
| 31 | 31 | var _ ports.SnapshotRepository = (*Repository)(nil) | |
| 32 | 32 | ||
| 33 | - | // NewRepository creates a repository rooted at dir. It does not create files | |
| 34 | - | // until the first publication, so merely constructing it is side-effect free. | |
| 35 | - | func NewRepository(dir string, logs ...*slog.Logger) *Repository { | |
| 36 | - | log := slog.Default() | |
| 37 | - | if len(logs) > 0 && logs[0] != nil { | |
| 38 | - | log = logs[0] | |
| 33 | + | // NewRepository creates a repository rooted at dir with the default logger. It | |
| 34 | + | // does not create files until the first publication, so construction is side-effect free. | |
| 35 | + | func NewRepository(dir string) *Repository { | |
| 36 | + | return NewRepositoryWithLogger(dir, slog.Default()) | |
| 37 | + | } | |
| 38 | + | ||
| 39 | + | // NewRepositoryWithLogger creates a repository rooted at dir with log. | |
| 40 | + | func NewRepositoryWithLogger(dir string, log *slog.Logger) *Repository { | |
| 41 | + | if log == nil { | |
| 42 | + | log = slog.Default() | |
| 39 | 43 | } | |
| 40 | 44 | return &Repository{ | |
| 41 | 45 | dir: dir, | |
| @@ -30,12 +30,16 @@ type Repository struct { | |||||
| 30 | 30 | ||||
| 31 | var _ ports.SnapshotRepository = (*Repository)(nil) | 31 | var _ ports.SnapshotRepository = (*Repository)(nil) | ||
| 32 | 32 | ||||
| 33 | - | // NewRepository creates a repository rooted at dir. It does not create files | 33 | + | // NewRepository creates a repository rooted at dir with the default logger. It |
| 34 | - | // until the first publication, so merely constructing it is side-effect free. | 34 | + | // does not create files until the first publication, so construction is side-effect free. |
| 35 | - | func NewRepository(dir string, logs ...*slog.Logger) *Repository { | 35 | + | func NewRepository(dir string) *Repository { |
| 36 | - | log := slog.Default() | 36 | + | return NewRepositoryWithLogger(dir, slog.Default()) |
| 37 | - | if len(logs) > 0 && logs[0] != nil { | 37 | + | } |
| 38 | - | log = logs[0] | 38 | + | |
| 39 | + | // NewRepositoryWithLogger creates a repository rooted at dir with log. | |||
| 40 | + | func NewRepositoryWithLogger(dir string, log *slog.Logger) *Repository { | |||
| 41 | + | if log == nil { | |||
| 42 | + | log = slog.Default() | |||
| 39 | } | 43 | } | ||
| 40 | return &Repository{ | 44 | return &Repository{ | ||
| 41 | dir: dir, | 45 | dir: dir, | ||
M internal/app/run.go +6 -16
| @@ -575,7 +575,7 @@ func runDaemonOwnedWithLogger(ctx context.Context, log *slog.Logger) (retErr err | |||
| 575 | 575 | daemonOpts = append(daemonOpts, daemon.WithConfig(cfg)) | |
| 576 | 576 | daemonOpts = append(daemonOpts, daemon.WithBarScriptCommandRunner(shellcmd.New())) | |
| 577 | 577 | daemonOpts = append(daemonOpts, daemon.WithProcessInspector(platform.NewProcessInspector()), daemon.WithDirOrHome(platform.DirOrHome)) | |
| 578 | - | snapshotRepository := snapshotadapter.NewRepository(snapshotDir(), log) | |
| 578 | + | snapshotRepository := snapshotadapter.NewRepositoryWithLogger(snapshotDir(), log) | |
| 579 | 579 | daemonOpts = append(daemonOpts, daemon.WithSnapshotRepository(snapshotRepository)) | |
| 580 | 580 | noticeStore := noticefile.New(platform.StateDir()) | |
| 581 | 581 | daemonOpts = append(daemonOpts, daemon.WithNoticeStore(noticeStore)) | |
| @@ -1370,28 +1370,18 @@ func printSessions(w io.Writer, sessions []ports.SessionInfo) { | |||
| 1370 | 1370 | } | |
| 1371 | 1371 | ||
| 1372 | 1372 | func unreadableCatalogueError(stateDir string) error { | |
| 1373 | - | return fmt.Errorf("vev: durable session state at %s cannot be read and was left untouched.\n"+ | |
| 1373 | + | return fmt.Errorf("%w: vev: durable session state at %s cannot be read and was left untouched.\n"+ | |
| 1374 | 1374 | "vev does not erase it automatically. To start fresh, remove it:\n"+ | |
| 1375 | - | " rm -rf %s", stateDir, stateDir) | |
| 1375 | + | " rm -rf %s", persist.ErrCatalogueUnreadable, stateDir, stateDir) | |
| 1376 | 1376 | } | |
| 1377 | 1377 | ||
| 1378 | 1378 | func runOfflineNamedKill(ctx context.Context, name string) (retErr error) { | |
| 1379 | 1379 | stateDir := platform.StateDir() | |
| 1380 | - | records, err := persist.LoadReadOnly(stateDir) | |
| 1381 | - | if err != nil { | |
| 1382 | - | return unreadableCatalogueError(stateDir) | |
| 1383 | - | } | |
| 1384 | - | found := false | |
| 1385 | - | for _, record := range records { | |
| 1386 | - | if record.Name == name { | |
| 1387 | - | found = true | |
| 1388 | - | break | |
| 1389 | - | } | |
| 1390 | - | } | |
| 1391 | - | if !found { | |
| 1380 | + | if _, err := os.Stat(persist.StorePath(stateDir)); errors.Is(err, os.ErrNotExist) { | |
| 1392 | 1381 | return fmt.Errorf("vev: no such session: %s", name) | |
| 1382 | + | } else if err != nil { | |
| 1383 | + | return fmt.Errorf("vev: reading stored sessions: %w", err) | |
| 1393 | 1384 | } | |
| 1394 | - | ||
| 1395 | 1385 | repository := snapshotadapter.NewRepository(snapshotDir()) | |
| 1396 | 1386 | opened, err := openCatalogue(stateDir) | |
| 1397 | 1387 | if err != nil { | |
| @@ -575,7 +575,7 @@ func runDaemonOwnedWithLogger(ctx context.Context, log *slog.Logger) (retErr err | |||||
| 575 | daemonOpts = append(daemonOpts, daemon.WithConfig(cfg)) | 575 | daemonOpts = append(daemonOpts, daemon.WithConfig(cfg)) | ||
| 576 | daemonOpts = append(daemonOpts, daemon.WithBarScriptCommandRunner(shellcmd.New())) | 576 | daemonOpts = append(daemonOpts, daemon.WithBarScriptCommandRunner(shellcmd.New())) | ||
| 577 | daemonOpts = append(daemonOpts, daemon.WithProcessInspector(platform.NewProcessInspector()), daemon.WithDirOrHome(platform.DirOrHome)) | 577 | daemonOpts = append(daemonOpts, daemon.WithProcessInspector(platform.NewProcessInspector()), daemon.WithDirOrHome(platform.DirOrHome)) | ||
| 578 | - | snapshotRepository := snapshotadapter.NewRepository(snapshotDir(), log) | 578 | + | snapshotRepository := snapshotadapter.NewRepositoryWithLogger(snapshotDir(), log) |
| 579 | daemonOpts = append(daemonOpts, daemon.WithSnapshotRepository(snapshotRepository)) | 579 | daemonOpts = append(daemonOpts, daemon.WithSnapshotRepository(snapshotRepository)) | ||
| 580 | noticeStore := noticefile.New(platform.StateDir()) | 580 | noticeStore := noticefile.New(platform.StateDir()) | ||
| 581 | daemonOpts = append(daemonOpts, daemon.WithNoticeStore(noticeStore)) | 581 | daemonOpts = append(daemonOpts, daemon.WithNoticeStore(noticeStore)) | ||
| @@ -1370,28 +1370,18 @@ func printSessions(w io.Writer, sessions []ports.SessionInfo) { | |||||
| 1370 | } | 1370 | } | ||
| 1371 | 1371 | ||||
| 1372 | func unreadableCatalogueError(stateDir string) error { | 1372 | func unreadableCatalogueError(stateDir string) error { | ||
| 1373 | - | return fmt.Errorf("vev: durable session state at %s cannot be read and was left untouched.\n"+ | 1373 | + | return fmt.Errorf("%w: vev: durable session state at %s cannot be read and was left untouched.\n"+ |
| 1374 | "vev does not erase it automatically. To start fresh, remove it:\n"+ | 1374 | "vev does not erase it automatically. To start fresh, remove it:\n"+ | ||
| 1375 | - | " rm -rf %s", stateDir, stateDir) | 1375 | + | " rm -rf %s", persist.ErrCatalogueUnreadable, stateDir, stateDir) |
| 1376 | } | 1376 | } | ||
| 1377 | 1377 | ||||
| 1378 | func runOfflineNamedKill(ctx context.Context, name string) (retErr error) { | 1378 | func runOfflineNamedKill(ctx context.Context, name string) (retErr error) { | ||
| 1379 | stateDir := platform.StateDir() | 1379 | stateDir := platform.StateDir() | ||
| 1380 | - | records, err := persist.LoadReadOnly(stateDir) | 1380 | + | if _, err := os.Stat(persist.StorePath(stateDir)); errors.Is(err, os.ErrNotExist) { |
| 1381 | - | if err != nil { | |||
| 1382 | - | return unreadableCatalogueError(stateDir) | |||
| 1383 | - | } | |||
| 1384 | - | found := false | |||
| 1385 | - | for _, record := range records { | |||
| 1386 | - | if record.Name == name { | |||
| 1387 | - | found = true | |||
| 1388 | - | break | |||
| 1389 | - | } | |||
| 1390 | - | } | |||
| 1391 | - | if !found { | |||
| 1392 | return fmt.Errorf("vev: no such session: %s", name) | 1381 | return fmt.Errorf("vev: no such session: %s", name) | ||
| 1382 | + | } else if err != nil { | |||
| 1383 | + | return fmt.Errorf("vev: reading stored sessions: %w", err) | |||
| 1393 | } | 1384 | } | ||
| 1394 | - | ||||
| 1395 | repository := snapshotadapter.NewRepository(snapshotDir()) | 1385 | repository := snapshotadapter.NewRepository(snapshotDir()) | ||
| 1396 | opened, err := openCatalogue(stateDir) | 1386 | opened, err := openCatalogue(stateDir) | ||
| 1397 | if err != nil { | 1387 | if err != nil { | ||
M internal/app/run_test.go +3 -2
| @@ -25,6 +25,7 @@ import ( | |||
| 25 | 25 | "github.com/bnema/vev/internal/usecase/confirm" | |
| 26 | 26 | "github.com/bnema/vev/internal/usecase/daemon" | |
| 27 | 27 | "github.com/bnema/vev/pkg/kv" | |
| 28 | + | "github.com/bnema/vev/pkg/safedir" | |
| 28 | 29 | "github.com/stretchr/testify/mock" | |
| 29 | 30 | "github.com/stretchr/testify/require" | |
| 30 | 31 | ) | |
| @@ -334,7 +335,7 @@ func TestLifecycleOwnershipPrecedesDaemonStartup(t *testing.T) { | |||
| 334 | 335 | runtimeRoot, stateRoot := t.TempDir(), t.TempDir() | |
| 335 | 336 | runtimeDir := filepath.Join(runtimeRoot, "vev") | |
| 336 | 337 | stateDir := filepath.Join(stateRoot, "vev") | |
| 337 | - | require.NoError(t, os.Mkdir(stateDir, 0o700)) | |
| 338 | + | require.NoError(t, safedir.EnsurePrivate(stateDir)) | |
| 338 | 339 | store, err := kv.Open(persist.StorePath(stateDir)) | |
| 339 | 340 | require.NoError(t, err) | |
| 340 | 341 | require.NoError(t, store.Set([]byte("work"), []byte("malformed catalogue value"))) | |
| @@ -727,7 +728,7 @@ func TestRunListUnreadableCatalogueProvidesResetGuidance(t *testing.T) { | |||
| 727 | 728 | runtimeDir := filepath.Join(runtimeRoot, "vev") | |
| 728 | 729 | t.Setenv("XDG_STATE_HOME", stateRoot) | |
| 729 | 730 | t.Setenv("XDG_RUNTIME_DIR", runtimeRoot) | |
| 730 | - | require.NoError(t, os.Mkdir(stateDir, 0o700)) | |
| 731 | + | require.NoError(t, safedir.EnsurePrivate(stateDir)) | |
| 731 | 732 | ||
| 732 | 733 | catalogue := persist.StorePath(stateDir) | |
| 733 | 734 | before := []byte("corrupt catalogue") | |
| @@ -25,6 +25,7 @@ import ( | |||||
| 25 | "github.com/bnema/vev/internal/usecase/confirm" | 25 | "github.com/bnema/vev/internal/usecase/confirm" | ||
| 26 | "github.com/bnema/vev/internal/usecase/daemon" | 26 | "github.com/bnema/vev/internal/usecase/daemon" | ||
| 27 | "github.com/bnema/vev/pkg/kv" | 27 | "github.com/bnema/vev/pkg/kv" | ||
| 28 | + | "github.com/bnema/vev/pkg/safedir" | |||
| 28 | "github.com/stretchr/testify/mock" | 29 | "github.com/stretchr/testify/mock" | ||
| 29 | "github.com/stretchr/testify/require" | 30 | "github.com/stretchr/testify/require" | ||
| 30 | ) | 31 | ) | ||
| @@ -334,7 +335,7 @@ func TestLifecycleOwnershipPrecedesDaemonStartup(t *testing.T) { | |||||
| 334 | runtimeRoot, stateRoot := t.TempDir(), t.TempDir() | 335 | runtimeRoot, stateRoot := t.TempDir(), t.TempDir() | ||
| 335 | runtimeDir := filepath.Join(runtimeRoot, "vev") | 336 | runtimeDir := filepath.Join(runtimeRoot, "vev") | ||
| 336 | stateDir := filepath.Join(stateRoot, "vev") | 337 | stateDir := filepath.Join(stateRoot, "vev") | ||
| 337 | - | require.NoError(t, os.Mkdir(stateDir, 0o700)) | 338 | + | require.NoError(t, safedir.EnsurePrivate(stateDir)) |
| 338 | store, err := kv.Open(persist.StorePath(stateDir)) | 339 | store, err := kv.Open(persist.StorePath(stateDir)) | ||
| 339 | require.NoError(t, err) | 340 | require.NoError(t, err) | ||
| 340 | require.NoError(t, store.Set([]byte("work"), []byte("malformed catalogue value"))) | 341 | require.NoError(t, store.Set([]byte("work"), []byte("malformed catalogue value"))) | ||
| @@ -727,7 +728,7 @@ func TestRunListUnreadableCatalogueProvidesResetGuidance(t *testing.T) { | |||||
| 727 | runtimeDir := filepath.Join(runtimeRoot, "vev") | 728 | runtimeDir := filepath.Join(runtimeRoot, "vev") | ||
| 728 | t.Setenv("XDG_STATE_HOME", stateRoot) | 729 | t.Setenv("XDG_STATE_HOME", stateRoot) | ||
| 729 | t.Setenv("XDG_RUNTIME_DIR", runtimeRoot) | 730 | t.Setenv("XDG_RUNTIME_DIR", runtimeRoot) | ||
| 730 | - | require.NoError(t, os.Mkdir(stateDir, 0o700)) | 731 | + | require.NoError(t, safedir.EnsurePrivate(stateDir)) |
| 731 | 732 | ||||
| 732 | catalogue := persist.StorePath(stateDir) | 733 | catalogue := persist.StorePath(stateDir) | ||
| 733 | before := []byte("corrupt catalogue") | 734 | before := []byte("corrupt catalogue") | ||
M internal/persist/catalogue_test.go +2 -1
| @@ -9,6 +9,7 @@ import ( | |||
| 9 | 9 | ||
| 10 | 10 | "github.com/bnema/vev/internal/domain" | |
| 11 | 11 | "github.com/bnema/vev/pkg/kv" | |
| 12 | + | "github.com/bnema/vev/pkg/safedir" | |
| 12 | 13 | ) | |
| 13 | 14 | ||
| 14 | 15 | func TestCatalogue(t *testing.T) { | |
| @@ -87,7 +88,7 @@ func testCatalogueOpenFailsClosed(t *testing.T) { | |||
| 87 | 88 | _, _, err := openCurrentCatalogue(dir, false) | |
| 88 | 89 | require.Error(t, err) | |
| 89 | 90 | ||
| 90 | - | require.NoError(t, os.MkdirAll(dir, 0o700)) | |
| 91 | + | require.NoError(t, safedir.EnsurePrivate(dir)) | |
| 91 | 92 | raw := []byte("corrupt catalogue") | |
| 92 | 93 | require.NoError(t, os.WriteFile(StorePath(dir), raw, 0o600)) | |
| 93 | 94 | _, _, err = openCurrentCatalogue(dir, false) | |
| @@ -9,6 +9,7 @@ import ( | |||||
| 9 | 9 | ||||
| 10 | "github.com/bnema/vev/internal/domain" | 10 | "github.com/bnema/vev/internal/domain" | ||
| 11 | "github.com/bnema/vev/pkg/kv" | 11 | "github.com/bnema/vev/pkg/kv" | ||
| 12 | + | "github.com/bnema/vev/pkg/safedir" | |||
| 12 | ) | 13 | ) | ||
| 13 | 14 | ||||
| 14 | func TestCatalogue(t *testing.T) { | 15 | func TestCatalogue(t *testing.T) { | ||
| @@ -87,7 +88,7 @@ func testCatalogueOpenFailsClosed(t *testing.T) { | |||||
| 87 | _, _, err := openCurrentCatalogue(dir, false) | 88 | _, _, err := openCurrentCatalogue(dir, false) | ||
| 88 | require.Error(t, err) | 89 | require.Error(t, err) | ||
| 89 | 90 | ||||
| 90 | - | require.NoError(t, os.MkdirAll(dir, 0o700)) | 91 | + | require.NoError(t, safedir.EnsurePrivate(dir)) |
| 91 | raw := []byte("corrupt catalogue") | 92 | raw := []byte("corrupt catalogue") | ||
| 92 | require.NoError(t, os.WriteFile(StorePath(dir), raw, 0o600)) | 93 | require.NoError(t, os.WriteFile(StorePath(dir), raw, 0o600)) | ||
| 93 | _, _, err = openCurrentCatalogue(dir, false) | 94 | _, _, err = openCurrentCatalogue(dir, false) | ||
M internal/persist/persist.go +8 -2
| @@ -143,7 +143,10 @@ func LoadCatalogueReadOnly(dir string) ([]domain.CatalogueRecord, error) { | |||
| 143 | 143 | ||
| 144 | 144 | func (p *Persister) Save(record domain.CatalogueRecord) error { return p.Replace(record.Name, record) } | |
| 145 | 145 | func (p *Persister) Create(record domain.CatalogueRecord) error { | |
| 146 | - | if p == nil || p.store == nil { | |
| 146 | + | if p == nil { | |
| 147 | + | return nil | |
| 148 | + | } | |
| 149 | + | if p.store == nil { | |
| 147 | 150 | return errPersistenceUnavailable | |
| 148 | 151 | } | |
| 149 | 152 | p.mu.Lock() | |
| @@ -159,7 +162,10 @@ func (p *Persister) Create(record domain.CatalogueRecord) error { | |||
| 159 | 162 | ||
| 160 | 163 | func (p *Persister) Records() ([]domain.CatalogueRecord, error) { return p.LoadCatalogue() } | |
| 161 | 164 | func (p *Persister) Record(name string) (domain.CatalogueRecord, bool, error) { | |
| 162 | - | if p == nil || p.store == nil { | |
| 165 | + | if p == nil { | |
| 166 | + | return domain.CatalogueRecord{}, false, nil | |
| 167 | + | } | |
| 168 | + | if p.store == nil { | |
| 163 | 169 | return domain.CatalogueRecord{}, false, errPersistenceUnavailable | |
| 164 | 170 | } | |
| 165 | 171 | p.mu.Lock() | |
| @@ -143,7 +143,10 @@ func LoadCatalogueReadOnly(dir string) ([]domain.CatalogueRecord, error) { | |||||
| 143 | 143 | ||||
| 144 | func (p *Persister) Save(record domain.CatalogueRecord) error { return p.Replace(record.Name, record) } | 144 | func (p *Persister) Save(record domain.CatalogueRecord) error { return p.Replace(record.Name, record) } | ||
| 145 | func (p *Persister) Create(record domain.CatalogueRecord) error { | 145 | func (p *Persister) Create(record domain.CatalogueRecord) error { | ||
| 146 | - | if p == nil || p.store == nil { | 146 | + | if p == nil { |
| 147 | + | return nil | |||
| 148 | + | } | |||
| 149 | + | if p.store == nil { | |||
| 147 | return errPersistenceUnavailable | 150 | return errPersistenceUnavailable | ||
| 148 | } | 151 | } | ||
| 149 | p.mu.Lock() | 152 | p.mu.Lock() | ||
| @@ -159,7 +162,10 @@ func (p *Persister) Create(record domain.CatalogueRecord) error { | |||||
| 159 | 162 | ||||
| 160 | func (p *Persister) Records() ([]domain.CatalogueRecord, error) { return p.LoadCatalogue() } | 163 | func (p *Persister) Records() ([]domain.CatalogueRecord, error) { return p.LoadCatalogue() } | ||
| 161 | func (p *Persister) Record(name string) (domain.CatalogueRecord, bool, error) { | 164 | func (p *Persister) Record(name string) (domain.CatalogueRecord, bool, error) { | ||
| 162 | - | if p == nil || p.store == nil { | 165 | + | if p == nil { |
| 166 | + | return domain.CatalogueRecord{}, false, nil | |||
| 167 | + | } | |||
| 168 | + | if p.store == nil { | |||
| 163 | return domain.CatalogueRecord{}, false, errPersistenceUnavailable | 169 | return domain.CatalogueRecord{}, false, errPersistenceUnavailable | ||
| 164 | } | 170 | } | ||
| 165 | p.mu.Lock() | 171 | p.mu.Lock() | ||
M internal/persist/persist_test.go +6 -5
| @@ -11,6 +11,7 @@ import ( | |||
| 11 | 11 | ||
| 12 | 12 | "github.com/bnema/vev/internal/domain" | |
| 13 | 13 | portsmocks "github.com/bnema/vev/internal/ports/mocks" | |
| 14 | + | "github.com/bnema/vev/pkg/safedir" | |
| 14 | 15 | ) | |
| 15 | 16 | ||
| 16 | 17 | func privateDir(t *testing.T) string { t.Helper(); return filepath.Join(t.TempDir(), "vev") } | |
| @@ -35,7 +36,7 @@ func TestOpenOrCreate(t *testing.T) { | |||
| 35 | 36 | t.Parallel() | |
| 36 | 37 | dir := privateDir(t) | |
| 37 | 38 | if tt.write != nil { | |
| 38 | - | require.NoError(t, os.MkdirAll(dir, 0o700)) | |
| 39 | + | require.NoError(t, safedir.EnsurePrivate(dir)) | |
| 39 | 40 | require.NoError(t, os.WriteFile(StorePath(dir), tt.write, 0o600)) | |
| 40 | 41 | } | |
| 41 | 42 | got, err := OpenOrCreate(dir) | |
| @@ -71,7 +72,7 @@ func TestOpenOrCreateRejectsNonCurrentDurableStateWithoutMutation(t *testing.T) | |||
| 71 | 72 | t.Run(tt.name, func(t *testing.T) { | |
| 72 | 73 | t.Parallel() | |
| 73 | 74 | dir := privateDir(t) | |
| 74 | - | require.NoError(t, os.MkdirAll(dir, 0o700)) | |
| 75 | + | require.NoError(t, safedir.EnsurePrivate(dir)) | |
| 75 | 76 | for name, data := range tt.files { | |
| 76 | 77 | require.NoError(t, os.WriteFile(filepath.Join(dir, name), data, 0o600)) | |
| 77 | 78 | } | |
| @@ -299,11 +300,11 @@ func (s *failClosedStore) CloseWithoutSync() error { | |||
| 299 | 300 | } | |
| 300 | 301 | ||
| 301 | 302 | func copyStoreData(data map[string][]byte) map[string][]byte { | |
| 302 | - | copy := make(map[string][]byte, len(data)) | |
| 303 | + | cloned := make(map[string][]byte, len(data)) | |
| 303 | 304 | for key, value := range data { | |
| 304 | - | copy[key] = append([]byte(nil), value...) | |
| 305 | + | cloned[key] = append([]byte(nil), value...) | |
| 305 | 306 | } | |
| 306 | - | return copy | |
| 307 | + | return cloned | |
| 307 | 308 | } | |
| 308 | 309 | ||
| 309 | 310 | func TestRejectedIdentityWriteFencesCatalogue(t *testing.T) { | |
| @@ -11,6 +11,7 @@ import ( | |||||
| 11 | 11 | ||||
| 12 | "github.com/bnema/vev/internal/domain" | 12 | "github.com/bnema/vev/internal/domain" | ||
| 13 | portsmocks "github.com/bnema/vev/internal/ports/mocks" | 13 | portsmocks "github.com/bnema/vev/internal/ports/mocks" | ||
| 14 | + | "github.com/bnema/vev/pkg/safedir" | |||
| 14 | ) | 15 | ) | ||
| 15 | 16 | ||||
| 16 | func privateDir(t *testing.T) string { t.Helper(); return filepath.Join(t.TempDir(), "vev") } | 17 | func privateDir(t *testing.T) string { t.Helper(); return filepath.Join(t.TempDir(), "vev") } | ||
| @@ -35,7 +36,7 @@ func TestOpenOrCreate(t *testing.T) { | |||||
| 35 | t.Parallel() | 36 | t.Parallel() | ||
| 36 | dir := privateDir(t) | 37 | dir := privateDir(t) | ||
| 37 | if tt.write != nil { | 38 | if tt.write != nil { | ||
| 38 | - | require.NoError(t, os.MkdirAll(dir, 0o700)) | 39 | + | require.NoError(t, safedir.EnsurePrivate(dir)) |
| 39 | require.NoError(t, os.WriteFile(StorePath(dir), tt.write, 0o600)) | 40 | require.NoError(t, os.WriteFile(StorePath(dir), tt.write, 0o600)) | ||
| 40 | } | 41 | } | ||
| 41 | got, err := OpenOrCreate(dir) | 42 | got, err := OpenOrCreate(dir) | ||
| @@ -71,7 +72,7 @@ func TestOpenOrCreateRejectsNonCurrentDurableStateWithoutMutation(t *testing.T) | |||||
| 71 | t.Run(tt.name, func(t *testing.T) { | 72 | t.Run(tt.name, func(t *testing.T) { | ||
| 72 | t.Parallel() | 73 | t.Parallel() | ||
| 73 | dir := privateDir(t) | 74 | dir := privateDir(t) | ||
| 74 | - | require.NoError(t, os.MkdirAll(dir, 0o700)) | 75 | + | require.NoError(t, safedir.EnsurePrivate(dir)) |
| 75 | for name, data := range tt.files { | 76 | for name, data := range tt.files { | ||
| 76 | require.NoError(t, os.WriteFile(filepath.Join(dir, name), data, 0o600)) | 77 | require.NoError(t, os.WriteFile(filepath.Join(dir, name), data, 0o600)) | ||
| 77 | } | 78 | } | ||
| @@ -299,11 +300,11 @@ func (s *failClosedStore) CloseWithoutSync() error { | |||||
| 299 | } | 300 | } | ||
| 300 | 301 | ||||
| 301 | func copyStoreData(data map[string][]byte) map[string][]byte { | 302 | func copyStoreData(data map[string][]byte) map[string][]byte { | ||
| 302 | - | copy := make(map[string][]byte, len(data)) | 303 | + | cloned := make(map[string][]byte, len(data)) |
| 303 | for key, value := range data { | 304 | for key, value := range data { | ||
| 304 | - | copy[key] = append([]byte(nil), value...) | 305 | + | cloned[key] = append([]byte(nil), value...) |
| 305 | } | 306 | } | ||
| 306 | - | return copy | 307 | + | return cloned |
| 307 | } | 308 | } | ||
| 308 | 309 | ||||
| 309 | func TestRejectedIdentityWriteFencesCatalogue(t *testing.T) { | 310 | func TestRejectedIdentityWriteFencesCatalogue(t *testing.T) { | ||
M internal/usecase/command/command_test.go +5 -0
| @@ -16,6 +16,11 @@ func TestParsePositiveUint64(t *testing.T) { | |||
| 16 | 16 | {name: "overflow", args: []string{"18446744073709551616"}, invalid: true}, | |
| 17 | 17 | {name: "zero", args: []string{"0"}, invalid: true}, | |
| 18 | 18 | {name: "leading zero", args: []string{"01"}, invalid: true}, | |
| 19 | + | {name: "empty string", args: []string{""}, invalid: true}, | |
| 20 | + | {name: "positive sign", args: []string{"+1"}, invalid: true}, | |
| 21 | + | {name: "negative sign", args: []string{"-1"}, invalid: true}, | |
| 22 | + | {name: "whitespace", args: []string{"1 2"}, invalid: true}, | |
| 23 | + | {name: "non-digit", args: []string{"1x"}, invalid: true}, | |
| 19 | 24 | {name: "empty arguments", invalid: true}, | |
| 20 | 25 | {name: "multiple arguments", args: []string{"1", "2"}, invalid: true}, | |
| 21 | 26 | } { | |
| @@ -16,6 +16,11 @@ func TestParsePositiveUint64(t *testing.T) { | |||||
| 16 | {name: "overflow", args: []string{"18446744073709551616"}, invalid: true}, | 16 | {name: "overflow", args: []string{"18446744073709551616"}, invalid: true}, | ||
| 17 | {name: "zero", args: []string{"0"}, invalid: true}, | 17 | {name: "zero", args: []string{"0"}, invalid: true}, | ||
| 18 | {name: "leading zero", args: []string{"01"}, invalid: true}, | 18 | {name: "leading zero", args: []string{"01"}, invalid: true}, | ||
| 19 | + | {name: "empty string", args: []string{""}, invalid: true}, | |||
| 20 | + | {name: "positive sign", args: []string{"+1"}, invalid: true}, | |||
| 21 | + | {name: "negative sign", args: []string{"-1"}, invalid: true}, | |||
| 22 | + | {name: "whitespace", args: []string{"1 2"}, invalid: true}, | |||
| 23 | + | {name: "non-digit", args: []string{"1x"}, invalid: true}, | |||
| 19 | {name: "empty arguments", invalid: true}, | 24 | {name: "empty arguments", invalid: true}, | ||
| 20 | {name: "multiple arguments", args: []string{"1", "2"}, invalid: true}, | 25 | {name: "multiple arguments", args: []string{"1", "2"}, invalid: true}, | ||
| 21 | } { | 26 | } { | ||
M internal/usecase/daemon/coderabbit_regression_test.go +39 -0
| @@ -109,6 +109,45 @@ func TestFinishRecordRestoreAlwaysClosesProvidedBarrier(t *testing.T) { | |||
| 109 | 109 | } | |
| 110 | 110 | } | |
| 111 | 111 | ||
| 112 | + | type blockingRecordCatalogue struct { | |
| 113 | + | ports.Catalogue | |
| 114 | + | record domain.CatalogueRecord | |
| 115 | + | entered chan struct{} | |
| 116 | + | release chan struct{} | |
| 117 | + | } | |
| 118 | + | ||
| 119 | + | func (c *blockingRecordCatalogue) Record(string) (domain.CatalogueRecord, bool, error) { | |
| 120 | + | close(c.entered) | |
| 121 | + | <-c.release | |
| 122 | + | return c.record, true, nil | |
| 123 | + | } | |
| 124 | + | ||
| 125 | + | func TestCreateSessionRechecksShutdownAfterCatalogueRead(t *testing.T) { | |
| 126 | + | record := durableRecoveryRecord(0) | |
| 127 | + | catalogue := &blockingRecordCatalogue{record: record, entered: make(chan struct{}), release: make(chan struct{})} | |
| 128 | + | d := newTestDaemon(t, nil, stubClock{}) | |
| 129 | + | d.catalogue = catalogue | |
| 130 | + | d.persistEnabled = true | |
| 131 | + | d.stopped[record.Name] = stoppedSessionFromRecord(record, ports.SessionStopped, make(chan struct{})) | |
| 132 | + | ||
| 133 | + | result := make(chan error, 1) | |
| 134 | + | go func() { | |
| 135 | + | _, err := createSessionForTest(d, record.Name, false, "/tmp", domain.Size{Cols: 80, Rows: 24}, terminalEnv{}, nil) | |
| 136 | + | result <- err | |
| 137 | + | }() | |
| 138 | + | <-catalogue.entered | |
| 139 | + | d.mu.Lock() | |
| 140 | + | d.closing = true | |
| 141 | + | d.mu.Unlock() | |
| 142 | + | close(catalogue.release) | |
| 143 | + | ||
| 144 | + | err := <-result | |
| 145 | + | require.ErrorContains(t, err, "daemon is shutting down") | |
| 146 | + | d.mu.Lock() | |
| 147 | + | defer d.mu.Unlock() | |
| 148 | + | require.Empty(t, d.sessions) | |
| 149 | + | } | |
| 150 | + | ||
| 112 | 151 | func TestSnapshotStopContextCancelIsIdempotent(t *testing.T) { | |
| 113 | 152 | deadline := &snapshotShutdownDeadline{done: make(chan struct{})} | |
| 114 | 153 | _, cancel := snapshotStopContext(deadline) | |
| @@ -109,6 +109,45 @@ func TestFinishRecordRestoreAlwaysClosesProvidedBarrier(t *testing.T) { | |||||
| 109 | } | 109 | } | ||
| 110 | } | 110 | } | ||
| 111 | 111 | ||||
| 112 | + | type blockingRecordCatalogue struct { | |||
| 113 | + | ports.Catalogue | |||
| 114 | + | record domain.CatalogueRecord | |||
| 115 | + | entered chan struct{} | |||
| 116 | + | release chan struct{} | |||
| 117 | + | } | |||
| 118 | + | ||||
| 119 | + | func (c *blockingRecordCatalogue) Record(string) (domain.CatalogueRecord, bool, error) { | |||
| 120 | + | close(c.entered) | |||
| 121 | + | <-c.release | |||
| 122 | + | return c.record, true, nil | |||
| 123 | + | } | |||
| 124 | + | ||||
| 125 | + | func TestCreateSessionRechecksShutdownAfterCatalogueRead(t *testing.T) { | |||
| 126 | + | record := durableRecoveryRecord(0) | |||
| 127 | + | catalogue := &blockingRecordCatalogue{record: record, entered: make(chan struct{}), release: make(chan struct{})} | |||
| 128 | + | d := newTestDaemon(t, nil, stubClock{}) | |||
| 129 | + | d.catalogue = catalogue | |||
| 130 | + | d.persistEnabled = true | |||
| 131 | + | d.stopped[record.Name] = stoppedSessionFromRecord(record, ports.SessionStopped, make(chan struct{})) | |||
| 132 | + | ||||
| 133 | + | result := make(chan error, 1) | |||
| 134 | + | go func() { | |||
| 135 | + | _, err := createSessionForTest(d, record.Name, false, "/tmp", domain.Size{Cols: 80, Rows: 24}, terminalEnv{}, nil) | |||
| 136 | + | result <- err | |||
| 137 | + | }() | |||
| 138 | + | <-catalogue.entered | |||
| 139 | + | d.mu.Lock() | |||
| 140 | + | d.closing = true | |||
| 141 | + | d.mu.Unlock() | |||
| 142 | + | close(catalogue.release) | |||
| 143 | + | ||||
| 144 | + | err := <-result | |||
| 145 | + | require.ErrorContains(t, err, "daemon is shutting down") | |||
| 146 | + | d.mu.Lock() | |||
| 147 | + | defer d.mu.Unlock() | |||
| 148 | + | require.Empty(t, d.sessions) | |||
| 149 | + | } | |||
| 150 | + | ||||
| 112 | func TestSnapshotStopContextCancelIsIdempotent(t *testing.T) { | 151 | func TestSnapshotStopContextCancelIsIdempotent(t *testing.T) { | ||
| 113 | deadline := &snapshotShutdownDeadline{done: make(chan struct{})} | 152 | deadline := &snapshotShutdownDeadline{done: make(chan struct{})} | ||
| 114 | _, cancel := snapshotStopContext(deadline) | 153 | _, cancel := snapshotStopContext(deadline) | ||
M internal/usecase/daemon/durable_recovery_test.go +8 -4
| @@ -266,11 +266,15 @@ func TestCatalogueRestoreIndependent(t *testing.T) { | |||
| 266 | 266 | ||
| 267 | 267 | d.restoreCatalogue(context.Background(), mustDurableRecords(t, catalogue)) | |
| 268 | 268 | ||
| 269 | - | require.Equal(t, ports.SessionStopped, d.stopped[healthy.Name].state) | |
| 270 | - | require.Equal(t, ports.SessionBroken, d.stopped[broken.Name].state) | |
| 271 | - | for _, name := range []string{healthy.Name, broken.Name} { | |
| 269 | + | d.mu.Lock() | |
| 270 | + | healthyEntry := d.stopped[healthy.Name] | |
| 271 | + | brokenEntry := d.stopped[broken.Name] | |
| 272 | + | d.mu.Unlock() | |
| 273 | + | require.Equal(t, ports.SessionStopped, healthyEntry.state) | |
| 274 | + | require.Equal(t, ports.SessionBroken, brokenEntry.state) | |
| 275 | + | for name, done := range map[string]<-chan struct{}{healthy.Name: healthyEntry.restoreDone, broken.Name: brokenEntry.restoreDone} { | |
| 272 | 276 | select { | |
| 273 | - | case <-d.stopped[name].restoreDone: | |
| 277 | + | case <-done: | |
| 274 | 278 | default: | |
| 275 | 279 | t.Fatalf("restore completion for %q was not closed", name) | |
| 276 | 280 | } | |
| @@ -266,11 +266,15 @@ func TestCatalogueRestoreIndependent(t *testing.T) { | |||||
| 266 | 266 | ||||
| 267 | d.restoreCatalogue(context.Background(), mustDurableRecords(t, catalogue)) | 267 | d.restoreCatalogue(context.Background(), mustDurableRecords(t, catalogue)) | ||
| 268 | 268 | ||||
| 269 | - | require.Equal(t, ports.SessionStopped, d.stopped[healthy.Name].state) | 269 | + | d.mu.Lock() |
| 270 | - | require.Equal(t, ports.SessionBroken, d.stopped[broken.Name].state) | 270 | + | healthyEntry := d.stopped[healthy.Name] |
| 271 | - | for _, name := range []string{healthy.Name, broken.Name} { | 271 | + | brokenEntry := d.stopped[broken.Name] |
| 272 | + | d.mu.Unlock() | |||
| 273 | + | require.Equal(t, ports.SessionStopped, healthyEntry.state) | |||
| 274 | + | require.Equal(t, ports.SessionBroken, brokenEntry.state) | |||
| 275 | + | for name, done := range map[string]<-chan struct{}{healthy.Name: healthyEntry.restoreDone, broken.Name: brokenEntry.restoreDone} { | |||
| 272 | select { | 276 | select { | ||
| 273 | - | case <-d.stopped[name].restoreDone: | 277 | + | case <-done: |
| 274 | default: | 278 | default: | ||
| 275 | t.Fatalf("restore completion for %q was not closed", name) | 279 | t.Fatalf("restore completion for %q was not closed", name) | ||
| 276 | } | 280 | } | ||
M internal/usecase/daemon/picker_test.go +5 -2
| @@ -353,8 +353,11 @@ func TestPickerRejectsCatalogueTargetsWithoutFreshRuntime(t *testing.T) { | |||
| 353 | 353 | sets := storeState.sets | |
| 354 | 354 | storeState.mu.Unlock() | |
| 355 | 355 | require.Zero(t, sets, "unsafe target must not mutate the catalogue") | |
| 356 | - | require.Equal(t, record, d.stopped[record.Name].record) | |
| 357 | - | require.Equal(t, tt.runtimeState, d.stopped[record.Name].state) | |
| 356 | + | d.mu.Lock() | |
| 357 | + | entry := d.stopped[record.Name] | |
| 358 | + | d.mu.Unlock() | |
| 359 | + | require.Equal(t, record, entry.record) | |
| 360 | + | require.Equal(t, tt.runtimeState, entry.state) | |
| 358 | 361 | }) | |
| 359 | 362 | } | |
| 360 | 363 | } | |
| @@ -353,8 +353,11 @@ func TestPickerRejectsCatalogueTargetsWithoutFreshRuntime(t *testing.T) { | |||||
| 353 | sets := storeState.sets | 353 | sets := storeState.sets | ||
| 354 | storeState.mu.Unlock() | 354 | storeState.mu.Unlock() | ||
| 355 | require.Zero(t, sets, "unsafe target must not mutate the catalogue") | 355 | require.Zero(t, sets, "unsafe target must not mutate the catalogue") | ||
| 356 | - | require.Equal(t, record, d.stopped[record.Name].record) | 356 | + | d.mu.Lock() |
| 357 | - | require.Equal(t, tt.runtimeState, d.stopped[record.Name].state) | 357 | + | entry := d.stopped[record.Name] |
| 358 | + | d.mu.Unlock() | |||
| 359 | + | require.Equal(t, record, entry.record) | |||
| 360 | + | require.Equal(t, tt.runtimeState, entry.state) | |||
| 358 | }) | 361 | }) | ||
| 359 | } | 362 | } | ||
| 360 | } | 363 | } | ||
M internal/usecase/daemon/session.go +4 -0
| @@ -201,6 +201,10 @@ func (d *Daemon) createSessionLocked(name string, ephemeral bool, cwd string, sz | |||
| 201 | 201 | if err != nil { | |
| 202 | 202 | return nil, domain.UserErr(domain.NoticeSessionSpawn, "couldn't read session catalogue", err) | |
| 203 | 203 | } | |
| 204 | + | if d.closing { | |
| 205 | + | return nil, &protoErr{ports.ErrServerShutdown, "daemon is shutting down"} | |
| 206 | + | } | |
| 207 | + | stopped, resuming = d.stopped[name] | |
| 204 | 208 | } | |
| 205 | 209 | var createdAt int64 | |
| 206 | 210 | var incarnation domain.IncarnationID | |
| @@ -201,6 +201,10 @@ func (d *Daemon) createSessionLocked(name string, ephemeral bool, cwd string, sz | |||||
| 201 | if err != nil { | 201 | if err != nil { | ||
| 202 | return nil, domain.UserErr(domain.NoticeSessionSpawn, "couldn't read session catalogue", err) | 202 | return nil, domain.UserErr(domain.NoticeSessionSpawn, "couldn't read session catalogue", err) | ||
| 203 | } | 203 | } | ||
| 204 | + | if d.closing { | |||
| 205 | + | return nil, &protoErr{ports.ErrServerShutdown, "daemon is shutting down"} | |||
| 206 | + | } | |||
| 207 | + | stopped, resuming = d.stopped[name] | |||
| 204 | } | 208 | } | ||
| 205 | var createdAt int64 | 209 | var createdAt int64 | ||
| 206 | var incarnation domain.IncarnationID | 210 | var incarnation domain.IncarnationID | ||
M internal/usecase/daemon/snapshot_test.go +3 -2
| @@ -16,8 +16,7 @@ import ( | |||
| 16 | 16 | "github.com/bnema/vev/pkg/renderer" | |
| 17 | 17 | ) | |
| 18 | 18 | ||
| 19 | - | // TestSnapshotWorkerPublishesContentAddressedCapture verifies that all new | |
| 20 | - | // checkpoints use the repository publication contract. | |
| 19 | + | // TestWithSnapshotRepositoryRejectsTypedNil verifies typed nil repositories are disabled. | |
| 21 | 20 | func TestWithSnapshotRepositoryRejectsTypedNil(t *testing.T) { | |
| 22 | 21 | var repository *snapshotAcceptanceRepository | |
| 23 | 22 | d := newTestDaemon(t, portsmocks.NewMockPTYFactory(t), stubClock{}) | |
| @@ -60,6 +59,8 @@ func TestCheckpointCatalogueFailureKeepsCaptureRetryable(t *testing.T) { | |||
| 60 | 59 | require.True(t, sess.snapDirty.Load(), "failed catalogue commit must remain retryable") | |
| 61 | 60 | } | |
| 62 | 61 | ||
| 62 | + | // TestSnapshotWorkerPublishesContentAddressedCapture verifies that all new | |
| 63 | + | // checkpoints use the repository publication contract. | |
| 63 | 64 | func TestSnapshotWorkerPublishesContentAddressedCapture(t *testing.T) { | |
| 64 | 65 | d := newTestDaemon(t, portsmocks.NewMockPTYFactory(t), stubClock{}) | |
| 65 | 66 | repository := portsmocks.NewMockSnapshotRepository(t) | |
| @@ -16,8 +16,7 @@ import ( | |||||
| 16 | "github.com/bnema/vev/pkg/renderer" | 16 | "github.com/bnema/vev/pkg/renderer" | ||
| 17 | ) | 17 | ) | ||
| 18 | 18 | ||||
| 19 | - | // TestSnapshotWorkerPublishesContentAddressedCapture verifies that all new | 19 | + | // TestWithSnapshotRepositoryRejectsTypedNil verifies typed nil repositories are disabled. |
| 20 | - | // checkpoints use the repository publication contract. | |||
| 21 | func TestWithSnapshotRepositoryRejectsTypedNil(t *testing.T) { | 20 | func TestWithSnapshotRepositoryRejectsTypedNil(t *testing.T) { | ||
| 22 | var repository *snapshotAcceptanceRepository | 21 | var repository *snapshotAcceptanceRepository | ||
| 23 | d := newTestDaemon(t, portsmocks.NewMockPTYFactory(t), stubClock{}) | 22 | d := newTestDaemon(t, portsmocks.NewMockPTYFactory(t), stubClock{}) | ||
| @@ -60,6 +59,8 @@ func TestCheckpointCatalogueFailureKeepsCaptureRetryable(t *testing.T) { | |||||
| 60 | require.True(t, sess.snapDirty.Load(), "failed catalogue commit must remain retryable") | 59 | require.True(t, sess.snapDirty.Load(), "failed catalogue commit must remain retryable") | ||
| 61 | } | 60 | } | ||
| 62 | 61 | ||||
| 62 | + | // TestSnapshotWorkerPublishesContentAddressedCapture verifies that all new | |||
| 63 | + | // checkpoints use the repository publication contract. | |||
| 63 | func TestSnapshotWorkerPublishesContentAddressedCapture(t *testing.T) { | 64 | func TestSnapshotWorkerPublishesContentAddressedCapture(t *testing.T) { | ||
| 64 | d := newTestDaemon(t, portsmocks.NewMockPTYFactory(t), stubClock{}) | 65 | d := newTestDaemon(t, portsmocks.NewMockPTYFactory(t), stubClock{}) | ||
| 65 | repository := portsmocks.NewMockSnapshotRepository(t) | 66 | repository := portsmocks.NewMockSnapshotRepository(t) | ||
M internal/usecase/recovery/checkpoint.go +5 -1
| @@ -37,8 +37,12 @@ func (c *Coordinator) PublishCheckpoint(ctx context.Context, name string, public | |||
| 37 | 37 | if !ok { | |
| 38 | 38 | return domain.CatalogueRecord{}, ErrCheckpointRecordNotFound | |
| 39 | 39 | } | |
| 40 | + | currentGeneration := uint64(0) | |
| 41 | + | if record.Committed != nil { | |
| 42 | + | currentGeneration = record.Committed.Generation | |
| 43 | + | } | |
| 40 | 44 | if publication.Name != name || publication.Name != record.Name || publication.IncarnationID != record.IncarnationID || | |
| 41 | - | publication.Generation == 0 || len(publication.Manifest) == 0 || | |
| 45 | + | publication.Generation <= currentGeneration || len(publication.Manifest) == 0 || | |
| 42 | 46 | !equalCheckpointRefs(publication.ParentCheckpoint, record.Committed) { | |
| 43 | 47 | return domain.CatalogueRecord{}, ErrCheckpointConflict | |
| 44 | 48 | } | |
| @@ -37,8 +37,12 @@ func (c *Coordinator) PublishCheckpoint(ctx context.Context, name string, public | |||||
| 37 | if !ok { | 37 | if !ok { | ||
| 38 | return domain.CatalogueRecord{}, ErrCheckpointRecordNotFound | 38 | return domain.CatalogueRecord{}, ErrCheckpointRecordNotFound | ||
| 39 | } | 39 | } | ||
| 40 | + | currentGeneration := uint64(0) | |||
| 41 | + | if record.Committed != nil { | |||
| 42 | + | currentGeneration = record.Committed.Generation | |||
| 43 | + | } | |||
| 40 | if publication.Name != name || publication.Name != record.Name || publication.IncarnationID != record.IncarnationID || | 44 | if publication.Name != name || publication.Name != record.Name || publication.IncarnationID != record.IncarnationID || | ||
| 41 | - | publication.Generation == 0 || len(publication.Manifest) == 0 || | 45 | + | publication.Generation <= currentGeneration || len(publication.Manifest) == 0 || |
| 42 | !equalCheckpointRefs(publication.ParentCheckpoint, record.Committed) { | 46 | !equalCheckpointRefs(publication.ParentCheckpoint, record.Committed) { | ||
| 43 | return domain.CatalogueRecord{}, ErrCheckpointConflict | 47 | return domain.CatalogueRecord{}, ErrCheckpointConflict | ||
| 44 | } | 48 | } | ||
M internal/usecase/recovery/checkpoint_test.go +21 -0
| @@ -3,6 +3,7 @@ package recovery | |||
| 3 | 3 | import ( | |
| 4 | 4 | "context" | |
| 5 | 5 | "errors" | |
| 6 | + | "fmt" | |
| 6 | 7 | "sync" | |
| 7 | 8 | "testing" | |
| 8 | 9 | ||
| @@ -144,6 +145,26 @@ func TestCheckpointCommits(t *testing.T) { | |||
| 144 | 145 | } | |
| 145 | 146 | } | |
| 146 | 147 | ||
| 148 | + | func TestCheckpointRejectsNonIncreasingGeneration(t *testing.T) { | |
| 149 | + | prior := checkpointRecord() | |
| 150 | + | priorRef := domain.CheckpointRef{Generation: 7, ManifestDigest: [32]byte{7}} | |
| 151 | + | prior.Committed = &priorRef | |
| 152 | + | ||
| 153 | + | for _, generation := range []uint64{7, 6} { | |
| 154 | + | t.Run(fmt.Sprintf("generation-%d", generation), func(t *testing.T) { | |
| 155 | + | catalogue := &checkpointCatalogue{record: prior} | |
| 156 | + | repository := &checkpointRepository{} | |
| 157 | + | coordinator := NewCoordinator(catalogue, repository, nil) | |
| 158 | + | ||
| 159 | + | _, err := coordinator.PublishCheckpoint(context.Background(), prior.Name, checkpointPublication(prior, generation)) | |
| 160 | + | ||
| 161 | + | require.ErrorIs(t, err, ErrCheckpointConflict) | |
| 162 | + | require.Empty(t, repository.publications) | |
| 163 | + | require.Zero(t, catalogue.replaces) | |
| 164 | + | }) | |
| 165 | + | } | |
| 166 | + | } | |
| 167 | + | ||
| 147 | 168 | func TestCheckpointCommitPublishOrphan(t *testing.T) { | |
| 148 | 169 | prior := checkpointRecord() | |
| 149 | 170 | priorRef := domain.CheckpointRef{Generation: 7, ManifestDigest: [32]byte{7}} | |
| @@ -3,6 +3,7 @@ package recovery | |||||
| 3 | import ( | 3 | import ( | ||
| 4 | "context" | 4 | "context" | ||
| 5 | "errors" | 5 | "errors" | ||
| 6 | + | "fmt" | |||
| 6 | "sync" | 7 | "sync" | ||
| 7 | "testing" | 8 | "testing" | ||
| 8 | 9 | ||||
| @@ -144,6 +145,26 @@ func TestCheckpointCommits(t *testing.T) { | |||||
| 144 | } | 145 | } | ||
| 145 | } | 146 | } | ||
| 146 | 147 | ||||
| 148 | + | func TestCheckpointRejectsNonIncreasingGeneration(t *testing.T) { | |||
| 149 | + | prior := checkpointRecord() | |||
| 150 | + | priorRef := domain.CheckpointRef{Generation: 7, ManifestDigest: [32]byte{7}} | |||
| 151 | + | prior.Committed = &priorRef | |||
| 152 | + | ||||
| 153 | + | for _, generation := range []uint64{7, 6} { | |||
| 154 | + | t.Run(fmt.Sprintf("generation-%d", generation), func(t *testing.T) { | |||
| 155 | + | catalogue := &checkpointCatalogue{record: prior} | |||
| 156 | + | repository := &checkpointRepository{} | |||
| 157 | + | coordinator := NewCoordinator(catalogue, repository, nil) | |||
| 158 | + | ||||
| 159 | + | _, err := coordinator.PublishCheckpoint(context.Background(), prior.Name, checkpointPublication(prior, generation)) | |||
| 160 | + | ||||
| 161 | + | require.ErrorIs(t, err, ErrCheckpointConflict) | |||
| 162 | + | require.Empty(t, repository.publications) | |||
| 163 | + | require.Zero(t, catalogue.replaces) | |||
| 164 | + | }) | |||
| 165 | + | } | |||
| 166 | + | } | |||
| 167 | + | ||||
| 147 | func TestCheckpointCommitPublishOrphan(t *testing.T) { | 168 | func TestCheckpointCommitPublishOrphan(t *testing.T) { | ||
| 148 | prior := checkpointRecord() | 169 | prior := checkpointRecord() | ||
| 149 | priorRef := domain.CheckpointRef{Generation: 7, ManifestDigest: [32]byte{7}} | 170 | priorRef := domain.CheckpointRef{Generation: 7, ManifestDigest: [32]byte{7}} | ||
M pkg/kv/kv_test.go +19 -6
| @@ -86,9 +86,22 @@ func TestOpenFailsClosedWithoutMutation(t *testing.T) { | |||
| 86 | 86 | }{ | |
| 87 | 87 | {name: "empty", raw: []byte{}}, | |
| 88 | 88 | {name: "short magic", raw: []byte("VE")}, | |
| 89 | - | {name: "unknown magic", raw: []byte("NOPE\x00\x03\x00\x00\x00\x00\x00\x00\x00\x00")}, | |
| 90 | - | {name: "unknown version", raw: func() []byte { b := validEmptyFile(); binary.BigEndian.PutUint16(b[4:6], 99); return b }()}, | |
| 91 | - | {name: "truncated entry", raw: append(validEmptyFile()[:10], 0, 0, 0, 1)}, | |
| 89 | + | {name: "unknown magic", raw: func() []byte { | |
| 90 | + | payload := append([]byte(nil), validEmptyFile()[:filePrefixLen]...) | |
| 91 | + | copy(payload[:4], "NOPE") | |
| 92 | + | return appendCRC(payload) | |
| 93 | + | }()}, | |
| 94 | + | {name: "unknown version", raw: func() []byte { | |
| 95 | + | payload := append([]byte(nil), validEmptyFile()[:filePrefixLen]...) | |
| 96 | + | binary.BigEndian.PutUint16(payload[4:6], 99) | |
| 97 | + | return appendCRC(payload) | |
| 98 | + | }()}, | |
| 99 | + | {name: "truncated entry", raw: func() []byte { | |
| 100 | + | payload := append([]byte(nil), validEmptyFile()[:filePrefixLen]...) | |
| 101 | + | binary.BigEndian.PutUint32(payload[6:10], 1) | |
| 102 | + | payload = append(payload, 0, 0, 0, 1) | |
| 103 | + | return appendCRC(payload) | |
| 104 | + | }()}, | |
| 92 | 105 | } | |
| 93 | 106 | for _, tt := range tests { | |
| 94 | 107 | t.Run(tt.name, func(t *testing.T) { | |
| @@ -185,9 +198,9 @@ func TestDecodeRejectsDuplicateKeysAndTrailingData(t *testing.T) { | |||
| 185 | 198 | _, err := decodeFile(raw) | |
| 186 | 199 | require.ErrorIs(t, err, ErrCorrupt) | |
| 187 | 200 | ||
| 188 | - | valid := validEmptyFile() | |
| 189 | - | valid = append(valid, 0) | |
| 190 | - | _, err = decodeFile(valid) | |
| 201 | + | payload = append([]byte(nil), validEmptyFile()[:filePrefixLen]...) | |
| 202 | + | payload = append(payload, 0) | |
| 203 | + | _, err = decodeFile(appendCRC(payload)) | |
| 191 | 204 | require.True(t, errors.Is(err, ErrCorrupt)) | |
| 192 | 205 | } | |
| 193 | 206 | ||
| @@ -86,9 +86,22 @@ func TestOpenFailsClosedWithoutMutation(t *testing.T) { | |||||
| 86 | }{ | 86 | }{ | ||
| 87 | {name: "empty", raw: []byte{}}, | 87 | {name: "empty", raw: []byte{}}, | ||
| 88 | {name: "short magic", raw: []byte("VE")}, | 88 | {name: "short magic", raw: []byte("VE")}, | ||
| 89 | - | {name: "unknown magic", raw: []byte("NOPE\x00\x03\x00\x00\x00\x00\x00\x00\x00\x00")}, | 89 | + | {name: "unknown magic", raw: func() []byte { |
| 90 | - | {name: "unknown version", raw: func() []byte { b := validEmptyFile(); binary.BigEndian.PutUint16(b[4:6], 99); return b }()}, | 90 | + | payload := append([]byte(nil), validEmptyFile()[:filePrefixLen]...) |
| 91 | - | {name: "truncated entry", raw: append(validEmptyFile()[:10], 0, 0, 0, 1)}, | 91 | + | copy(payload[:4], "NOPE") |
| 92 | + | return appendCRC(payload) | |||
| 93 | + | }()}, | |||
| 94 | + | {name: "unknown version", raw: func() []byte { | |||
| 95 | + | payload := append([]byte(nil), validEmptyFile()[:filePrefixLen]...) | |||
| 96 | + | binary.BigEndian.PutUint16(payload[4:6], 99) | |||
| 97 | + | return appendCRC(payload) | |||
| 98 | + | }()}, | |||
| 99 | + | {name: "truncated entry", raw: func() []byte { | |||
| 100 | + | payload := append([]byte(nil), validEmptyFile()[:filePrefixLen]...) | |||
| 101 | + | binary.BigEndian.PutUint32(payload[6:10], 1) | |||
| 102 | + | payload = append(payload, 0, 0, 0, 1) | |||
| 103 | + | return appendCRC(payload) | |||
| 104 | + | }()}, | |||
| 92 | } | 105 | } | ||
| 93 | for _, tt := range tests { | 106 | for _, tt := range tests { | ||
| 94 | t.Run(tt.name, func(t *testing.T) { | 107 | t.Run(tt.name, func(t *testing.T) { | ||
| @@ -185,9 +198,9 @@ func TestDecodeRejectsDuplicateKeysAndTrailingData(t *testing.T) { | |||||
| 185 | _, err := decodeFile(raw) | 198 | _, err := decodeFile(raw) | ||
| 186 | require.ErrorIs(t, err, ErrCorrupt) | 199 | require.ErrorIs(t, err, ErrCorrupt) | ||
| 187 | 200 | ||||
| 188 | - | valid := validEmptyFile() | 201 | + | payload = append([]byte(nil), validEmptyFile()[:filePrefixLen]...) |
| 189 | - | valid = append(valid, 0) | 202 | + | payload = append(payload, 0) |
| 190 | - | _, err = decodeFile(valid) | 203 | + | _, err = decodeFile(appendCRC(payload)) |
| 191 | require.True(t, errors.Is(err, ErrCorrupt)) | 204 | require.True(t, errors.Is(err, ErrCorrupt)) | ||
| 192 | } | 205 | } | ||
| 193 | 206 | ||||