the directory lock is the kernel's, not a pid we believe

flock(2) on the lock file's descriptor replaces "write my pid, and
decide whether the pid in the file is still alive". The kernel drops
the lock when the process ends, however it ends, so there is no stale
krino lock to detect and no takeover to race over.

What that fixes:

- Two runs that both judged a lock stale could remove and recreate it
  and both believe they held it. Remove-then-create cannot be made
  atomic; there is nothing to make atomic now. Pinned by a test with
  eight callers over twenty rounds.
- A pid reused after a crash made the lock live for ever, and the
  message named neither the file nor the pid, so there was nothing to
  act on. The message now names both.
- Signal(0) reads EPERM as "not running", so a lock held by another
  user was taken over. There is no such judgement left to get wrong.

A run that waits for a held lock now says so first. Waiting is what
the spec asks for, but the wait has no timeout, and in silence it is
indistinguishable from a hang - I spent two minutes on one myself
today, waiting on a lock the window was holding.

Tests that faked a held lock by writing a file now hold a real one.

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)
 	}
 }