fix: review findings for lazysecrets list: recent means the last 90 days, cache saves lock and keep deletions and the TUI's cursors, failed lists aren't retried, one expiry rule

internal/cachefile/cachefile.go

@@ -32,6 +32,10 @@ type File struct {
 	Objects    map[string]Stamped[[]provider.Object]    `json:"objects,omitempty"`
 	Details    map[string]Stamped[provider.Object]      `json:"details,omitempty"`
 	Cursor     map[string]string                        `json:"cursor,omitempty"`
+	// Dropped are keys deleted since the file was read (a list that
+	// failed to load): Save deletes them from the file too, rather than
+	// merging the old entry back. Not saved.
+	Dropped map[string]bool `json:"-"`
 }
 
 // Stamped is a list or detail with when it was loaded.
@@ -69,6 +73,16 @@ func ProviderOf(key string) string {
 	return id
 }
 
+// Keep is Save's keep for these providers: their lists are kept, but not
+// repos', whose files are read from disk anyway, nor other accounts'.
+func Keep(providers []provider.Provider) func(providerID string) bool {
+	keep := map[string]bool{}
+	for _, p := range providers {
+		keep[p.ID()] = p.Kind() != provider.KindRepo
+	}
+	return func(id string) bool { return keep[id] }
+}
+
 // Read reads path; a missing, unreadable or other version's file is an
 // empty cache.
 func Read(path string) File {
@@ -90,17 +104,27 @@ func Read(path string) File {
 }
 
 // Save writes f to path, merged with the file there: of a list in both,
-// the one loaded last wins, and f's cursors win. Entries older than
-// MaxAge, and those of providers keep says no to (accounts signed out
-// of), are dropped, from the file's side as well.
+// the one loaded last wins, and f's cursors win (a nil Cursor keeps the
+// file's). Entries older than MaxAge, those of providers keep says no to
+// (accounts signed out of), and f.Dropped are dropped, from the file's
+// side as well. Saves from several lazysecrets take turns (a lock file),
+// so none undoes another's.
 func Save(path string, f File, keep func(providerID string) bool, now time.Time) error {
+	if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil {
+		return err
+	}
+	unlock, err := lock(path + ".lock")
+	if err != nil {
+		return err
+	}
+	defer unlock()
 	out := New()
 	ok := func(key string, at time.Time) bool { return keep(ProviderOf(key)) && now.Sub(at) < MaxAge }
 	disk := Read(path)
-	merge(out.Sources, disk.Sources, f.Sources, ok)
-	merge(out.Containers, disk.Containers, f.Containers, ok)
-	merge(out.Objects, disk.Objects, f.Objects, ok)
-	merge(out.Details, disk.Details, f.Details, ok)
+	merge(out.Sources, disk.Sources, f.Sources, ok, f.Dropped)
+	merge(out.Containers, disk.Containers, f.Containers, ok, f.Dropped)
+	merge(out.Objects, disk.Objects, f.Objects, ok, f.Dropped)
+	merge(out.Details, disk.Details, f.Details, ok, f.Dropped)
 	for _, m := range []map[string]string{disk.Cursor, f.Cursor} {
 		for k, v := range m {
 			if keep(ProviderOf(k)) {
@@ -111,12 +135,17 @@ func Save(path string, f File, keep func(providerID string) bool, now time.Time)
 	return write(path, out)
 }
 
-func merge[T any](out, disk, ours map[string]Stamped[T], ok func(string, time.Time) bool) {
-	for _, m := range []map[string]Stamped[T]{disk, ours} {
-		for k, e := range m {
-			if old, seen := out[k]; ok(k, e.At) && (!seen || !e.At.Before(old.At)) {
-				out[k] = e
-			}
+// merge puts disk's entries in out, but not those dropped, then ours
+// where they're newer.
+func merge[T any](out, disk, ours map[string]Stamped[T], ok func(string, time.Time) bool, dropped map[string]bool) {
+	for k, e := range disk {
+		if ok(k, e.At) && !dropped[k] {
+			out[k] = e
+		}
+	}
+	for k, e := range ours {
+		if old, seen := out[k]; ok(k, e.At) && (!seen || !e.At.Before(old.At)) {
+			out[k] = e
 		}
 	}
 }
@@ -127,9 +156,6 @@ func write(path string, f File) error {
 	if err != nil {
 		return err
 	}
-	if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil {
-		return err
-	}
 	tmp, err := os.CreateTemp(filepath.Dir(path), ".cache-*.json")
 	if err != nil {
 		return err

internal/cachefile/cachefile_test.go

@@ -89,3 +89,34 @@ func TestReadOtherVersion(t *testing.T) {
 		t.Errorf("read another version's file: %+v", got)
 	}
 }
+
+func TestSaveDroppedAndCursors(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "cache.json")
+	now := time.Now()
+	all := func(string) bool { return true }
+	f := New()
+	f.Containers["az|bad"] = list(now, "kv-old")
+	f.Cursor["az|x"] = "kv-tui"
+	if err := Save(path, f, all, now); err != nil {
+		t.Fatal(err)
+	}
+	// The TUI fails to load it again and drops it: it stays gone.
+	f = New()
+	f.Dropped = map[string]bool{"az|bad": true}
+	if err := Save(path, f, all, now); err != nil {
+		t.Fatal(err)
+	}
+	if _, ok := Read(path).Containers["az|bad"]; ok {
+		t.Error("a dropped list came back")
+	}
+	// lazysecrets list saves without cursors: the TUI's stay.
+	listed := New()
+	listed.Cursor = nil
+	listed.Containers["az|y"] = list(now, "kv-y")
+	if err := Save(path, listed, all, now); err != nil {
+		t.Fatal(err)
+	}
+	if Read(path).Cursor["az|x"] != "kv-tui" {
+		t.Error("the TUI's cursor was lost")
+	}
+}

internal/cachefile/lock_other.go

@@ -0,0 +1,7 @@
+//go:build !unix
+
+package cachefile
+
+// lock does nothing here: saves can then race, losing lists that load
+// again.
+func lock(string) (func(), error) { return func() {}, nil }

internal/cachefile/lock_unix.go

@@ -0,0 +1,25 @@
+//go:build unix
+
+package cachefile
+
+import (
+	"os"
+	"syscall"
+)
+
+// lock takes path as a lock file, waiting while another lazysecrets has
+// it, and returns how to let go.
+func lock(path string) (unlock func(), err error) {
+	f, err := os.OpenFile(path, os.O_CREATE|os.O_RDWR, 0o600)
+	if err != nil {
+		return nil, err
+	}
+	if err := syscall.Flock(int(f.Fd()), syscall.LOCK_EX); err != nil {
+		f.Close()
+		return nil, err
+	}
+	return func() {
+		_ = syscall.Flock(int(f.Fd()), syscall.LOCK_UN)
+		f.Close()
+	}, nil
+}