Fix Keychain popups on MacOS

app/secrets/secrets.go

@@ -2,6 +2,7 @@ package secrets
 
 import (
 	"errors"
+	"sync"
 
 	"github.com/0skillallluck/scanline/internal/g"
 )
@@ -10,8 +11,14 @@ var (
 	ErrKeyNotFound = errors.New("key not found")
 )
 
+// getService returns the platform secrets service wrapped in an in-memory cache.
+// The cache makes each unique key cost at most one keychain prompt per process,
+// which matters on macOS where unsigned binaries re-prompt on every access.
 var getService = g.Lazy(func() Service {
-	return newService()
+	return &cachedService{
+		inner: newService(),
+		items: make(map[string]cacheEntry),
+	}
 })
 
 type Service interface {
@@ -36,3 +43,85 @@ type ServiceError struct {
 func Healthcheck() *ServiceError {
 	return getService().Available()
 }
+
+type cacheEntry struct {
+	item  Item
+	found bool // false → memoized ErrKeyNotFound
+}
+
+type cachedService struct {
+	inner Service
+	mu    sync.RWMutex
+	items map[string]cacheEntry
+}
+
+func (c *cachedService) Available() *ServiceError {
+	return c.inner.Available()
+}
+
+func (c *cachedService) Get(key string) (Item, error) {
+	c.mu.RLock()
+	entry, ok := c.items[key]
+	c.mu.RUnlock()
+	if ok {
+		if !entry.found {
+			return Item{}, ErrKeyNotFound
+		}
+		return entry.item, nil
+	}
+
+	// Call the keychain unlocked so concurrent misses for distinct keys
+	// (e.g. RefreshServers reading one token per account) parallelize
+	// instead of serializing. Concurrent misses for the same key may both
+	// fetch — acceptable here since no caller reads the same key twice in
+	// parallel.
+	item, err := c.inner.Get(key)
+	if err == ErrKeyNotFound {
+		c.mu.Lock()
+		c.items[key] = cacheEntry{found: false}
+		c.mu.Unlock()
+		return Item{}, ErrKeyNotFound
+	}
+	if err != nil {
+		// Transient errors may resolve on retry; caching would mask recovery.
+		return Item{}, err
+	}
+	c.mu.Lock()
+	c.items[key] = cacheEntry{item: item, found: true}
+	c.mu.Unlock()
+	return item, nil
+}
+
+func (c *cachedService) Has(key string) (bool, error) {
+	_, err := c.Get(key)
+	if err == ErrKeyNotFound {
+		return false, nil
+	}
+	if err != nil {
+		return false, err
+	}
+	return true, nil
+}
+
+func (c *cachedService) Set(key string, value Item) error {
+	if err := c.inner.Set(key, value); err != nil {
+		return err
+	}
+	c.mu.Lock()
+	c.items[key] = cacheEntry{item: value, found: true}
+	c.mu.Unlock()
+	return nil
+}
+
+func (c *cachedService) Delete(key string) error {
+	err := c.inner.Delete(key)
+	c.mu.Lock()
+	if err == nil || err == ErrKeyNotFound {
+		c.items[key] = cacheEntry{found: false}
+	} else {
+		// Real error — invalidate so the next Get refetches.
+		delete(c.items, key)
+	}
+	c.mu.Unlock()
+	return err
+}

app/secrets/secrets_test.go

@@ -0,0 +1,206 @@
+package secrets
+
+import (
+	"errors"
+	"sync"
+	"sync/atomic"
+	"testing"
+)
+
+type fakeService struct {
+	mu    sync.Mutex
+	store map[string]Item
+	gets  int64
+	sets  int64
+	dels  int64
+	getErr error // injected error for the next Get if non-nil
+}
+
+func newFakeService() *fakeService {
+	return &fakeService{store: make(map[string]Item)}
+}
+
+func (f *fakeService) Available() *ServiceError { return nil }
+
+func (f *fakeService) Get(key string) (Item, error) {
+	atomic.AddInt64(&f.gets, 1)
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	if f.getErr != nil {
+		err := f.getErr
+		f.getErr = nil
+		return Item{}, err
+	}
+	item, ok := f.store[key]
+	if !ok {
+		return Item{}, ErrKeyNotFound
+	}
+	return item, nil
+}
+
+func (f *fakeService) Has(key string) (bool, error) {
+	_, err := f.Get(key)
+	if err == ErrKeyNotFound {
+		return false, nil
+	}
+	return err == nil, err
+}
+
+func (f *fakeService) Set(key string, value Item) error {
+	atomic.AddInt64(&f.sets, 1)
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	f.store[key] = value
+	return nil
+}
+
+func (f *fakeService) Delete(key string) error {
+	atomic.AddInt64(&f.dels, 1)
+	f.mu.Lock()
+	defer f.mu.Unlock()
+	if _, ok := f.store[key]; !ok {
+		return ErrKeyNotFound
+	}
+	delete(f.store, key)
+	return nil
+}
+
+func newCached(inner Service) *cachedService {
+	return &cachedService{inner: inner, items: make(map[string]cacheEntry)}
+}
+
+func TestCachedService_GetCachesHits(t *testing.T) {
+	fake := newFakeService()
+	fake.store["k"] = Item{Password: "v"}
+	cache := newCached(fake)
+
+	for i := 0; i < 5; i++ {
+		got, err := cache.Get("k")
+		if err != nil || got.Password != "v" {
+			t.Fatalf("Get #%d: got=%v err=%v", i, got, err)
+		}
+	}
+	if got := atomic.LoadInt64(&fake.gets); got != 1 {
+		t.Errorf("inner.Get called %d times, want 1", got)
+	}
+}
+
+func TestCachedService_GetCachesMisses(t *testing.T) {
+	fake := newFakeService()
+	cache := newCached(fake)
+
+	for i := 0; i < 5; i++ {
+		_, err := cache.Get("missing")
+		if err != ErrKeyNotFound {
+			t.Fatalf("Get #%d: err=%v, want ErrKeyNotFound", i, err)
+		}
+	}
+	if got := atomic.LoadInt64(&fake.gets); got != 1 {
+		t.Errorf("inner.Get called %d times, want 1 (negative-cache memoization)", got)
+	}
+}
+
+func TestCachedService_TransientErrorNotCached(t *testing.T) {
+	fake := newFakeService()
+	cache := newCached(fake)
+
+	fake.getErr = errors.New("boom")
+	if _, err := cache.Get("k"); err == nil {
+		t.Fatal("expected error")
+	}
+	// Next call should hit the inner service again (no cached error).
+	fake.store["k"] = Item{Password: "v"}
+	got, err := cache.Get("k")
+	if err != nil || got.Password != "v" {
+		t.Fatalf("got=%v err=%v", got, err)
+	}
+	if got := atomic.LoadInt64(&fake.gets); got != 2 {
+		t.Errorf("inner.Get called %d times, want 2", got)
+	}
+}
+
+func TestCachedService_SetIsWriteThrough(t *testing.T) {
+	fake := newFakeService()
+	cache := newCached(fake)
+
+	if err := cache.Set("k", Item{Password: "v"}); err != nil {
+		t.Fatal(err)
+	}
+	if got := atomic.LoadInt64(&fake.sets); got != 1 {
+		t.Errorf("inner.Set called %d times, want 1", got)
+	}
+	got, err := cache.Get("k")
+	if err != nil || got.Password != "v" {
+		t.Fatalf("got=%v err=%v", got, err)
+	}
+	if got := atomic.LoadInt64(&fake.gets); got != 0 {
+		t.Errorf("inner.Get called %d times after Set, want 0 (Set should populate cache)", got)
+	}
+}
+
+func TestCachedService_DeleteInvalidates(t *testing.T) {
+	fake := newFakeService()
+	fake.store["k"] = Item{Password: "v"}
+	cache := newCached(fake)
+
+	if _, err := cache.Get("k"); err != nil {
+		t.Fatal(err)
+	}
+	if err := cache.Delete("k"); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := cache.Get("k"); err != ErrKeyNotFound {
+		t.Fatalf("post-delete Get err=%v, want ErrKeyNotFound", err)
+	}
+	// inner.Get should only have been called once (initial); Delete memoizes
+	// not-found so the post-delete Get is a cache hit.
+	if got := atomic.LoadInt64(&fake.gets); got != 1 {
+		t.Errorf("inner.Get called %d times, want 1", got)
+	}
+}
+
+func TestCachedService_HasUsesCache(t *testing.T) {
+	fake := newFakeService()
+	fake.store["k"] = Item{Password: "v"}
+	cache := newCached(fake)
+
+	for i := 0; i < 3; i++ {
+		ok, err := cache.Has("k")
+		if err != nil || !ok {
+			t.Fatalf("Has #%d: ok=%v err=%v", i, ok, err)
+		}
+	}
+	if got := atomic.LoadInt64(&fake.gets); got != 1 {
+		t.Errorf("inner.Get called %d times, want 1", got)
+	}
+}
+
+func TestCachedService_ConcurrentDistinctKeys(t *testing.T) {
+	fake := newFakeService()
+	for i := 0; i < 32; i++ {
+		fake.store[key(i)] = Item{Password: "v"}
+	}
+	cache := newCached(fake)
+
+	var wg sync.WaitGroup
+	for i := 0; i < 32; i++ {
+		wg.Add(1)
+		go func(i int) {
+			defer wg.Done()
+			for j := 0; j < 100; j++ {
+				if _, err := cache.Get(key(i)); err != nil {
+					t.Errorf("Get(%s): %v", key(i), err)
+				}
+			}
+		}(i)
+	}
+	wg.Wait()
+
+	if got := atomic.LoadInt64(&fake.gets); got != 32 {
+		t.Errorf("inner.Get called %d times, want 32 (one per distinct key)", got)
+	}
+}
+
+func key(i int) string {
+	return string(rune('a'+i%26)) + string(rune('0'+i/26))
+}

app/secrets/service_darwin.go

@@ -8,7 +8,7 @@ import (
 )
 
 const (
-	serviceName = "dev.skillless.Scanless"
+	serviceName = "dev.skillless.Scanline"
 )
 
 type serviceDarwin struct{}
@@ -86,7 +86,11 @@ func (s *serviceDarwin) Set(key string, value Item) error {
 	item.SetAccount(key)
 	item.SetLabel(value.Label)
 	item.SetData([]byte(value.Password))
-	item.SetSynchronizable(keychain.AccessibleWhenUnlocked)
+	// SynchronizableNo is required — the macOS file-based keychain rejects
+	// SetAccessible alone with errSecParam. Net effect: device-local storage
+	// with access tied to keychain unlock state.
+	item.SetSynchronizable(keychain.SynchronizableNo)
+	item.SetAccessible(keychain.AccessibleWhenUnlocked)
 
 	if err := keychain.AddItem(item); err == keychain.ErrorDuplicateItem {
 		query := keychain.NewItem()