1 files changed, 95 insertions, 16 deletions

diff --git a/internal/lock/lock_test.go b/internal/lock/lock_test.go
index 04023aa..c756bb2 100644
--- a/internal/lock/lock_test.go
+++ b/internal/lock/lock_test.go
@@ -9,6 +9,7 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 )
@@ -106,32 +107,110 @@ func TestAcquireRespectsContextCancellation(t *testing.T) {
 	}
 }
 
-// TestStaleLockIsTakenOver: a lock naming a pid that is not running must not
-// wedge krino - a machine that lost power mid-run would need manual cleanup.
-func TestStaleLockIsTakenOver(t *testing.T) {
+// TestALockFileWithoutAHolderIsFree: a machine that lost power mid-run
+// leaves the lock file behind, but nothing holding it. The text in that
+// file is for a human; it must not wedge the next run, whatever it says.
+func TestALockFileWithoutAHolderIsFree(t *testing.T) {
 	path := filepath.Join(t.TempDir(), "dl.lock")
-	if err := os.WriteFile(path, []byte("pid 4294967000\nstarted 2020-01-01T00:00:00Z\n"), 0o644); err != nil {
+	if err := os.WriteFile(path, []byte("pid 1\nstarted 2020-01-01T00:00:00Z\n"), 0o644); err != nil {
 		t.Fatal(err)
 	}
 	l, err := Acquire(context.Background(), path, false)
 	if err != nil {
-		t.Fatalf("a stale lock blocked Acquire: %v", err)
+		t.Fatalf("a lock file nobody holds blocked Acquire: %v", err)
 	}
 	defer l.Release()
-	if !l.TookOverStale {
-		t.Error("the takeover was not reported to the caller")
+	if b, _ := os.ReadFile(path); !strings.Contains(string(b), fmt.Sprint(os.Getpid())) {
+		t.Errorf("the lock file still names the old holder: %q", b)
+	}
+}
+
+// TestALockDiesWithItsProcess: the kernel drops an flock when the last
+// descriptor closes, however the process ended. A holder that vanishes
+// without unlinking - a crash, a kill -9 - must leave the directory usable
+// at once. This is the reason the lock is the kernel's rather than a pid
+// written into a file and believed.
+func TestALockDiesWithItsProcess(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "dl.lock")
+	l, err := Acquire(context.Background(), path, false)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err := Acquire(context.Background(), path, false); !errors.Is(err, ErrHeld) {
+		t.Fatalf("while held, Acquire err = %v, want ErrHeld", err)
+	}
+	// Close the descriptor without unlinking, as a crash would leave it.
+	if err := l.f.Close(); err != nil {
+		t.Fatal(err)
+	}
+	l.f = nil
+	if _, err := os.Stat(path); err != nil {
+		t.Fatalf("the lock file should still be there: %v", err)
+	}
+	second, err := Acquire(context.Background(), path, false)
+	if err != nil {
+		t.Fatalf("a lock file left by a dead holder still blocks Acquire: %v", err)
 	}
+	second.Release()
 }
 
-// TestRunningRejectsPidsBeyondTheKernelsRange: a pid wider than 32 bits in a
-// damaged lock file is cut to its low bits by kill(2), so without a range
-// check it names some other process - here this very one - and a stale lock
-// would never be taken over.
-func TestRunningRejectsPidsBeyondTheKernelsRange(t *testing.T) {
-	if running(os.Getpid() + 1<<32) {
-		t.Error("running(pid + 2^32) is true: the pid was truncated to this process")
+// TestOnlyOneAcquireWinsAtOnce: the promise is that a directory is locked
+// while krino works in it. Judging a lock stale and recreating it cannot be
+// made atomic, so two runs reaching that conclusion together could both
+// believe they held it; the kernel's lock has one winner by construction.
+func TestOnlyOneAcquireWinsAtOnce(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "dl.lock")
+	for round := 0; round < 20; round++ {
+		const n = 8
+		start := make(chan struct{})
+		var mu sync.Mutex
+		var won []*Lock
+		var wg sync.WaitGroup
+		for i := 0; i < n; i++ {
+			wg.Add(1)
+			go func() {
+				defer wg.Done()
+				<-start
+				if l, err := Acquire(context.Background(), path, false); err == nil {
+					mu.Lock()
+					won = append(won, l)
+					mu.Unlock()
+				}
+			}()
+		}
+		close(start)
+		wg.Wait()
+		if len(won) != 1 {
+			t.Fatalf("round %d: %d callers hold the same lock at once, want 1", round, len(won))
+		}
+		for _, l := range won {
+			if err := l.Release(); err != nil {
+				t.Fatal(err)
+			}
+		}
+	}
+}
+
+// TestHeldErrorNamesTheLockAndHolder: "another krino is working in this
+// directory" leaves nothing to act on when the holder is a pid that was
+// reused after a crash - the lock is then live for ever as far as krino can
+// tell. The error must name the file to remove and the pid it blames.
+func TestHeldErrorNamesTheLockAndHolder(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "dl.lock")
+	first, err := Acquire(context.Background(), path, false)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer first.Release()
+
+	_, err = Acquire(context.Background(), path, false)
+	if !errors.Is(err, ErrHeld) {
+		t.Fatalf("second Acquire err = %v, want ErrHeld", err)
+	}
+	if got := err.Error(); !strings.Contains(got, path) {
+		t.Errorf("the error does not name the lock file %q: %s", path, got)
 	}
-	if !running(os.Getpid()) {
-		t.Error("running(own pid) is false")
+	if got := err.Error(); !strings.Contains(got, fmt.Sprint(os.Getpid())) {
+		t.Errorf("the error does not name the holder's pid %d: %s", os.Getpid(), got)
 	}
 }