fix(storage): refcount the process lock for same-process stores

storage/lock.go

@@ -0,0 +1,98 @@
+package storage
+
+import (
+	"fmt"
+	"os"
+	"sync"
+)
+
+// ProcessLock provides cross-process exclusive ownership of a database and
+// in-process reference counting of the lock. The OS lock (flock on Unix,
+// LockFileEx on Windows) is held once per process on a lock file next to
+// the database; multiple stores in the same process — common in hosts that
+// open the memory bridge from several callsites — share it via a refcount
+// and never block each other.
+//
+// The in-memory no-op lock (path == "") is safe to create and release.
+type ProcessLock struct {
+	proc *procLock
+	once sync.Once
+	err  error
+}
+
+// procLock is the per-path, per-process holder of the OS lock. It lives in
+// procHolders for as long as at least one ProcessLock references it.
+type procLock struct {
+	fd   *os.File
+	path string
+	refs int
+}
+
+var (
+	procMu      sync.Mutex
+	procHolders = make(map[string]*procLock)
+)
+
+// AcquireProcessLock returns a shared process lock for dbPath. The first
+// acquisition in the process takes the OS lock non-blockingly and fails if
+// another yaad process already holds it; subsequent acquisitions from the
+// same process bump the refcount. In-memory DSNs return a no-op lock.
+func AcquireProcessLock(dbPath string) (*ProcessLock, error) {
+	if isMemoryDSN(dbPath) {
+		return &ProcessLock{}, nil // no-op for in-memory
+	}
+	procMu.Lock()
+	defer procMu.Unlock()
+
+	lockPath := dbPath + ".lock"
+	if p, ok := procHolders[lockPath]; ok {
+		p.refs++
+		return &ProcessLock{proc: p}, nil
+	}
+
+	fd, err := os.OpenFile(lockPath, os.O_CREATE|os.O_RDWR, 0o600)
+	if err != nil {
+		return nil, fmt.Errorf("open lock file: %w", err)
+	}
+	// Non-blocking exclusive lock: fail fast instead of waiting for the
+	// other process to exit.
+	if err := acquireOSLock(fd); err != nil {
+		_ = fd.Close()
+		return nil, fmt.Errorf("database locked by another yaad process: %w", err)
+	}
+	p := &procLock{fd: fd, path: lockPath, refs: 1}
+	procHolders[lockPath] = p
+	return &ProcessLock{proc: p}, nil
+}
+
+// Release decrements the refcount and, on the last release in the process,
+// unlocks and best-effort removes the lock file. Repeated releases on the
+// same lock and releases of the no-op lock are safe.
+func (l *ProcessLock) Release() error {
+	if l.proc == nil {
+		return nil
+	}
+	l.once.Do(func() {
+		l.err = l.releaseOnce()
+	})
+	return l.err
+}
+
+func (l *ProcessLock) releaseOnce() error {
+	procMu.Lock()
+	defer procMu.Unlock()
+
+	// A refcount drop must stay paired with its holder: if another Release
+	// raced us for the same holder (guarded by l.once), refs is already
+	// accounted for.
+	p := l.proc
+	p.refs--
+	if p.refs > 0 {
+		return nil
+	}
+	delete(procHolders, p.path)
+	releaseOSLock(p.fd)
+	err := p.fd.Close()
+	_ = os.Remove(p.path) // best-effort cleanup
+	return err
+}

storage/lock_process_test.go

@@ -0,0 +1,127 @@
+package storage
+
+import (
+	"fmt"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// TestProcessLockSameProcessSharing is a regression test for hosts that open
+// several stores against one database within a single process (e.g. hawk's
+// YaadBridge is constructed from multiple callsites). The OS lock is shared
+// via reference counting instead of rejecting the second store.
+func TestProcessLockSameProcessSharing(t *testing.T) {
+	dir := t.TempDir()
+	dbPath := filepath.Join(dir, "shared.db")
+
+	stores := make([]*Store, 0, 4)
+	for i := 0; i < 4; i++ {
+		s, err := NewStore(dbPath)
+		if err != nil {
+			t.Fatalf("NewStore #%d in same process: %v", i+1, err)
+		}
+		stores = append(stores, s)
+	}
+
+	// Closing stores early must not release the OS lock while others hold it.
+	stores[0].Close()
+	stores[1].Close()
+
+	if s, err := NewStore(dbPath); err != nil {
+		t.Fatalf("NewStore while refcount > 0: %v", err)
+	} else {
+		stores = append(stores, s)
+	}
+
+	for _, s := range stores[2:] {
+		s.Close()
+	}
+}
+
+// TestProcessLockRefcountRelease verifies the lock file is only removed once
+// the last in-process holder releases, and that repeated Release calls are
+// safe.
+func TestProcessLockRefcountRelease(t *testing.T) {
+	dir := t.TempDir()
+	dbPath := filepath.Join(dir, "ref.db")
+	lockPath := dbPath + ".lock"
+
+	l1, err := AcquireProcessLock(dbPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	l2, err := AcquireProcessLock(dbPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if err := l1.Release(); err != nil {
+		t.Fatalf("Release #1: %v", err)
+	}
+	if err := l1.Release(); err != nil {
+		t.Fatalf("repeat Release must be safe: %v", err)
+	}
+	// l2 still holds the lock; the file must remain.
+	if _, err := os.Stat(lockPath); err != nil {
+		t.Errorf("lock file disappeared while a holder remains: %v", err)
+	}
+
+	if err := l2.Release(); err != nil {
+		t.Fatalf("Release #2: %v", err)
+	}
+	if _, err := os.Stat(lockPath); !os.IsNotExist(err) {
+		t.Errorf("lock file should be removed after last release, stat err = %v", err)
+	}
+}
+
+// TestProcessLockCrossProcess verifies that a store in a *separate* process
+// is rejected while this process holds the lock. The child re-runs this test
+// binary with the lock-held database path and prints a sentinel:
+// "LOCKBLOCKED" when NewStore fails (correct), "LOCKACQUIRED" if it ever
+// grabs the lock (bug). The parent asserts the child saw the block.
+func TestProcessLockCrossProcess(t *testing.T) {
+	if os.Getenv("YAAD_LOCK_CHILD") == "1" {
+		runLockChild()
+		return // unreachable: runLockChild exits
+	}
+
+	dir := t.TempDir()
+	dbPath := filepath.Join(dir, "cross.db")
+	s, err := NewStore(dbPath)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer s.Close()
+
+	cmd := exec.Command(os.Args[0], "-test.run=TestProcessLockCrossProcess")
+	cmd.Env = append(os.Environ(), "YAAD_LOCK_CHILD=1", "YAAD_LOCK_DB="+dbPath)
+	out, err := cmd.CombinedOutput()
+	got := string(out)
+
+	if err != nil {
+		t.Fatalf("child process error: %v\noutput:\n%s", err, got)
+	}
+	switch {
+	case containsLockSentinel(got, "LOCKACQUIRED"):
+		t.Fatalf("child acquired the lock while parent holds it:\n%s", got)
+	case !containsLockSentinel(got, "LOCKBLOCKED"):
+		t.Fatalf("child did not report being blocked by the lock:\n%s", got)
+	}
+}
+
+func containsLockSentinel(out, sentinel string) bool {
+	return strings.Contains(out, sentinel)
+}
+
+func runLockChild() {
+	dbPath := os.Getenv("YAAD_LOCK_DB")
+	if _, err := NewStore(dbPath); err == nil {
+		fmt.Println("LOCKACQUIRED")
+		os.Exit(1)
+	}
+	fmt.Println("LOCKBLOCKED")
+	os.Exit(0)
+}

storage/lock_unix.go

@@ -3,51 +3,17 @@
 package storage
 
 import (
-	"fmt"
 	"os"
 
 	"golang.org/x/sys/unix"
 )
 
-// ProcessLock provides cross-process advisory locking over the database via
-// flock(2) on a lock file next to the database. Holding the lock for the
-// lifetime of a Store guarantees a single yaad process owns a given
-// database file.
-type ProcessLock struct {
-	fd   *os.File
-	path string
+// acquireOSLock takes a non-blocking exclusive flock(2) on the lock file.
+func acquireOSLock(fd *os.File) error {
+	return unix.Flock(int(fd.Fd()), unix.LOCK_EX|unix.LOCK_NB)
 }
 
-// AcquireProcessLock acquires an exclusive, non-blocking lock on a lock file
-// next to the database. It returns an error if another yaad process already
-// holds the lock. In-memory DSNs return a no-op lock.
-func AcquireProcessLock(dbPath string) (*ProcessLock, error) {
-	if isMemoryDSN(dbPath) {
-		return &ProcessLock{}, nil // no-op for in-memory
-	}
-	lockPath := dbPath + ".lock"
-	fd, err := os.OpenFile(lockPath, os.O_CREATE|os.O_RDWR, 0o600)
-	if err != nil {
-		return nil, fmt.Errorf("open lock file: %w", err)
-	}
-	// Non-blocking exclusive lock: fail fast instead of waiting for the
-	// other process to exit.
-	if err := unix.Flock(int(fd.Fd()), unix.LOCK_EX|unix.LOCK_NB); err != nil {
-		_ = fd.Close()
-		return nil, fmt.Errorf("database locked by another yaad process: %w", err)
-	}
-	return &ProcessLock{fd: fd, path: lockPath}, nil
-}
-
-// Release releases the lock, closes the lock file, and best-effort removes
-// it from disk. Releasing a no-op (in-memory) lock is a nil error.
-func (l *ProcessLock) Release() error {
-	if l.fd == nil {
-		return nil
-	}
-	_ = unix.Flock(int(l.fd.Fd()), unix.LOCK_UN) // best-effort; Close releases anyway
-	err := l.fd.Close()
-	l.fd = nil
-	_ = os.Remove(l.path) // best-effort cleanup
-	return err
+// releaseOSLock releases the flock; a subsequent Close releases it anyway.
+func releaseOSLock(fd *os.File) {
+	_ = unix.Flock(int(fd.Fd()), unix.LOCK_UN)
 }

storage/lock_windows.go

@@ -3,62 +3,26 @@
 package storage
 
 import (
-	"fmt"
 	"os"
 
 	"golang.org/x/sys/windows"
 )
 
-// ProcessLock provides cross-process advisory locking over the database via
-// Windows LockFileEx on a lock file next to the database. Holding the lock
-// for the lifetime of a Store guarantees a single yaad process owns a given
-// database file.
-type ProcessLock struct {
-	fd   *os.File
-	path string
-}
-
-// AcquireProcessLock acquires an exclusive, non-blocking lock on a lock file
-// next to the database. It returns an error if another yaad process already
-// holds the lock. In-memory DSNs return a no-op lock.
-func AcquireProcessLock(dbPath string) (*ProcessLock, error) {
-	if isMemoryDSN(dbPath) {
-		return &ProcessLock{}, nil // no-op for in-memory
-	}
-	lockPath := dbPath + ".lock"
-	fd, err := os.OpenFile(lockPath, os.O_CREATE|os.O_RDWR, 0o600)
-	if err != nil {
-		return nil, fmt.Errorf("open lock file: %w", err)
-	}
+// acquireOSLock takes a non-blocking exclusive LockFileEx on the lock file.
+func acquireOSLock(fd *os.File) error {
 	handle := windows.Handle(fd.Fd())
-	// Non-blocking exclusive lock over the whole file: fail fast instead of
-	// waiting for the other process to exit.
-	err = windows.LockFileEx(
+	return windows.LockFileEx(
 		handle,
 		windows.LOCKFILE_EXCLUSIVE_LOCK|windows.LOCKFILE_FAIL_IMMEDIATELY,
 		0,
 		0xFFFFFFFF,
 		0xFFFFFFFF,
 		&windows.Overlapped{},
 	)
-	if err != nil {
-		_ = fd.Close()
-		return nil, fmt.Errorf("database locked by another yaad process: %w", err)
-	}
-	return &ProcessLock{fd: fd, path: lockPath}, nil
 }
 
-// Release releases the lock, closes the lock file, and best-effort removes
-// it from disk. Releasing a no-op (in-memory) lock is a nil error.
-func (l *ProcessLock) Release() error {
-	if l.fd == nil {
-		return nil
-	}
-	handle := windows.Handle(l.fd.Fd())
-	// Closing the handle releases the lock anyway; ignore unlock failure.
+// releaseOSLock releases the file lock; a subsequent Close releases it anyway.
+func releaseOSLock(fd *os.File) {
+	handle := windows.Handle(fd.Fd())
 	_ = windows.UnlockFileEx(handle, 0, 0xFFFFFFFF, 0xFFFFFFFF, &windows.Overlapped{})
-	err := l.fd.Close()
-	l.fd = nil
-	_ = os.Remove(l.path) // best-effort cleanup
-	return err
 }

storage/sqlite_test.go

@@ -503,20 +503,22 @@ func TestProcessLock(t *testing.T) {
 		t.Fatalf("NewStore #1: %v", err)
 	}
 
-	// Second store on same path should fail
+	// Second store on the same path succeeds in-process: the process lock
+	// is refcounted per path, so multiple stores in one process (the common
+	// case for hosts that open the memory bridge from several callsites)
+	// share the OS lock instead of blocking each other.
 	s2, err := NewStore(dbPath)
-	if err == nil {
-		s2.Close()
-		t.Fatal("expected error for second store on same DB, got nil")
-	}
-	if err.Error() == "" {
-		t.Fatal("expected error message")
+	if err != nil {
+		t.Fatalf("NewStore #2 (same process, refcounted): %v", err)
 	}
 
-	// Release first lock
+	// Release s1; s2 still holds the refcount, so the lock remains.
 	s1.Close()
 
-	// Now second should succeed
+	// s2 alone keeps the database openable across its own close.
+	s2.Close()
+
+	// After the last in-process release, a fresh store still succeeds.
 	s3, err := NewStore(dbPath)
 	if err != nil {
 		t.Fatalf("NewStore after release: %v", err)