From 212f7230bc3a0e45559c289414024e81582b4705 Mon Sep 17 00:00:00 2001 From: Lakshman Patel Date: Sat, 15 Aug 2026 18:46:03 +0530 Subject: [PATCH] fix(storage): refcount the process lock for same-process stores flock(2)/LockFileEx is per-process, so the merged PR #47 lock rejected a second NewStore on the same DB within one process. Hosts that construct the store from multiple callsites (hawk opens a YaadBridge per callsite) would then silently lose memory access. - Keep the OS lock exclusive across processes (flock / LockFileEx). - Within a process, share it per lock path via a refcount registry: Acquire bumps, Release unrefs; the file is unlocked and removed only on the last release. - Release is idempotent; in-memory no-op locks are unchanged. - Tests: same-process sharing, refcount release ordering, and a cross-process sentinel test asserting another process is still blocked. --- storage/lock.go | 98 +++++++++++++++++++++++++++ storage/lock_process_test.go | 127 +++++++++++++++++++++++++++++++++++ storage/lock_unix.go | 46 ++----------- storage/lock_windows.go | 48 ++----------- storage/sqlite_test.go | 20 +++--- 5 files changed, 248 insertions(+), 91 deletions(-) create mode 100644 storage/lock.go create mode 100644 storage/lock_process_test.go diff --git a/storage/lock.go b/storage/lock.go new file mode 100644 index 0000000..3e04eed --- /dev/null +++ b/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 +} diff --git a/storage/lock_process_test.go b/storage/lock_process_test.go new file mode 100644 index 0000000..462f228 --- /dev/null +++ b/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) +} diff --git a/storage/lock_unix.go b/storage/lock_unix.go index fbdefd0..ee263d9 100644 --- a/storage/lock_unix.go +++ b/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) } diff --git a/storage/lock_windows.go b/storage/lock_windows.go index 769258c..8e8d6d5 100644 --- a/storage/lock_windows.go +++ b/storage/lock_windows.go @@ -3,37 +3,15 @@ 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, @@ -41,24 +19,10 @@ func AcquireProcessLock(dbPath string) (*ProcessLock, error) { 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 } diff --git a/storage/sqlite_test.go b/storage/sqlite_test.go index 0321c38..7967ac6 100644 --- a/storage/sqlite_test.go +++ b/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)