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.

3 files changed, 58 insertions, 12 deletions

diff --git a/cmd/krino/history_test.go b/cmd/krino/history_test.go
index ad7f6f6..6277e7b 100644
--- a/cmd/krino/history_test.go
+++ b/cmd/krino/history_test.go
@@ -5,7 +5,6 @@ package main
 import (
 	"bytes"
 	"context"
-	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
@@ -14,6 +13,7 @@ import (
 
 	"git.labunix.xyz/krino/internal/engine"
 	"git.labunix.xyz/krino/internal/journal"
+	"git.labunix.xyz/krino/internal/lock"
 )
 
 func TestLogListsRunsAndUndoReverses(t *testing.T) {
@@ -101,12 +101,13 @@ func TestUndoFailsImmediatelyWithHeldLock(t *testing.T) {
 	}
 
 	held := filepath.Join(h, ".local", "state", "krino", "dl.lock")
-	if err := os.MkdirAll(filepath.Dir(held), 0o755); err != nil {
-		t.Fatal(err)
-	}
-	if err := os.WriteFile(held, []byte(fmt.Sprintf("pid %d\n", os.Getpid())), 0o644); err != nil {
+	// A real lock, not a file that looks like one: the lock is the
+	// kernel's, so holding it means holding a descriptor.
+	l, err := lock.Acquire(context.Background(), held, false)
+	if err != nil {
 		t.Fatal(err)
 	}
+	defer l.Release()
 
 	code, out, errOut := runCLI(t, "undo", "-y")
 	if code != 1 || !strings.Contains(errOut, "another krino") {
@@ -560,3 +561,31 @@ func TestIgnoredGlobalFlagsAreRefused(t *testing.T) {
 		t.Errorf("log -n 3 (its own count flag) was refused: %d %q", code, errOut)
 	}
 }
+
+// TestWaitingForALockSaysSo: every run but -y waits for a held lock, which
+// the spec intends (§3, §11) - but waiting silently is indistinguishable
+// from a hang, and the wait has no timeout. A run about to wait must say
+// what it is waiting for before it blocks.
+func TestWaitingForALockSaysSo(t *testing.T) {
+	h := matchingFixture(t)
+	held := filepath.Join(h, ".local", "state", "krino", "dl.lock")
+	l, err := lock.Acquire(context.Background(), held, false)
+	if err != nil {
+		t.Fatal(err)
+	}
+	go func() {
+		time.Sleep(250 * time.Millisecond)
+		l.Release()
+	}()
+
+	code, _, errOut := runCLI(t, "-n")
+	if code != 0 {
+		t.Fatalf("dry run after the lock was released: %d %s", code, errOut)
+	}
+	if !strings.Contains(errOut, "waiting") {
+		t.Errorf("a run that waited for the lock said nothing about it:\n%q", errOut)
+	}
+	if !strings.Contains(errOut, held) {
+		t.Errorf("the notice does not name the lock file %q:\n%q", held, errOut)
+	}
+}
diff --git a/cmd/krino/matching_test.go b/cmd/krino/matching_test.go
index 4ce2205..6bdc994 100644
--- a/cmd/krino/matching_test.go
+++ b/cmd/krino/matching_test.go
@@ -4,13 +4,15 @@ package main
 
 import (
 	"bytes"
+	"context"
 	"encoding/json"
-	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
 	"testing"
 	"time"
+
+	"git.labunix.xyz/krino/internal/lock"
 )
 
 const dlRules = `
@@ -412,13 +414,13 @@ func TestRefusesWithoutTerminalAndWithoutFlags(t *testing.T) {
 func TestSecondRunFailsImmediatelyWithYes(t *testing.T) {
 	h := matchingFixture(t)
 	held := filepath.Join(h, ".local", "state", "krino", "dl.lock")
-	if err := os.MkdirAll(filepath.Dir(held), 0o755); err != nil {
-		t.Fatal(err)
-	}
-	// A lock held by this very process, so it is not stale.
-	if err := os.WriteFile(held, []byte(fmt.Sprintf("pid %d\n", os.Getpid())), 0o644); err != nil {
+	// A lock really held: it is the kernel's, so a file that merely looks
+	// like one holds nothing.
+	l, err := lock.Acquire(context.Background(), held, false)
+	if err != nil {
 		t.Fatal(err)
 	}
+	defer l.Release()
 	code, _, errOut := runCLI(t, "-y")
 	if code != 1 || !strings.Contains(errOut, "another krino") {
 		t.Errorf("-y against a held lock: %d %q", code, errOut)
diff --git a/cmd/krino/sort.go b/cmd/krino/sort.go
index 35d8123..a381239 100644
--- a/cmd/krino/sort.go
+++ b/cmd/krino/sort.go
@@ -18,6 +18,7 @@ import (
 	"golang.org/x/term"
 
 	"git.labunix.xyz/krino/internal/engine"
+	"git.labunix.xyz/krino/internal/lock"
 	"git.labunix.xyz/krino/internal/plan"
 	"git.labunix.xyz/krino/internal/scan"
 	"git.labunix.xyz/krino/internal/xdg"
@@ -126,7 +127,7 @@ func cmdSort(g *globals, names []string, stdout, stderr io.Writer) int {
 		// behind a stuck run. lock.Acquire takes ctx precisely so that wait
 		// is not unbounded in practice: a signal cancels it and Acquire
 		// returns ctx.Err() promptly instead of polling forever.
-		l, err := sess.Lock(ctx, d, !g.yes)
+		l, err := lockDir(ctx, sess, d, !g.yes, stderr)
 		if err != nil {
 			if interrupted(err) {
 				// Interrupted while waiting for the lock: an interrupt, not
@@ -602,3 +603,17 @@ func interrupted(err error) bool {
 func stopAfterApply(action rune, err error) bool {
 	return interrupted(err) || action == 'w'
 }
+
+// lockDir takes d's lock, saying out loud what a silent wait would hide.
+// The lock is tried without waiting first: when it is held and this run may
+// wait, the holder is named before the wait begins, so a run that is
+// waiting does not look like a run that has hung - there is no timeout on
+// the wait, and until it prints something the two are indistinguishable.
+func lockDir(ctx context.Context, sess *engine.Session, d *engine.Dir, wait bool, stderr io.Writer) (*lock.Lock, error) {
+	l, err := sess.Lock(ctx, d, false)
+	if wait && errors.Is(err, lock.ErrHeld) {
+		fmt.Fprintf(stderr, "krino: %s: %v; waiting\n", d.Name, err)
+		return sess.Lock(ctx, d, true)
+	}
+	return l, err
+}