fix(cli): a credentials save stamps its own file's mtime before it truncates, so the empty-file memo holds off Linux (BUG-3838) (#2203)

* fix(cli): a credentials save sets the mtime before it truncates, so the empty-file memo holds off Linux (BUG-3838)

BUG-3829's emptyConfirmed memo assumed a save's truncate moves an already
empty file's mtime. That was measured on ext4 only; POSIX moves mtime on
ftruncate only when the size changes, and macOS and Windows were never
measured. On such a platform a process that had memoized an empty file could
answer empty during another process's unlocked save and lose an entry.

writeFileInPlace0600 now sets the mtime itself (os.Chtimes, best effort like
the chmod) before the truncate, so the memo no longer depends on the
truncate's side effect. loadStoreLocked also reads once more when the stat
after its last empty read shows a body (hunter r6 on BUG-3829).

Claude-Session: https://claude.ai/code/session_012QxKpjqXTpU84auY5zZ8FR

* fix(cli): on Windows the save touches its own handle, not the path; the stalled-save test releases on failure (BUG-3838)

Codex r1 could not establish that os.Chtimes works on an open file on
Windows. It does not, safely: UtimesNano opens the path again with only
FILE_SHARE_WRITE, so it fails against a reader holding the file and, while
open, makes a concurrent reader's open fail. touchCredentialsFile now uses
SetFileTime on the handle the save already holds (GENERIC_WRITE includes
FILE_WRITE_ATTRIBUTES); other platforms keep os.Chtimes by path.

The test's stalled save is released and waited for in cleanup, so a failure
before the release leaves no goroutine in the seam (codex r1 Low).

Claude-Session: https://claude.ai/code/session_012QxKpjqXTpU84auY5zZ8FR

* test(cli): the stalled-save test's wait for the write is bounded (BUG-3838)

A save that failed after its release left the read seam spinning until the
global test timeout (hunter r2). It now gives up after five seconds and fails;
cleanup reports the save's error.

Claude-Session: https://claude.ai/code/session_012QxKpjqXTpU84auY5zZ8FR

* fix(cli): on Unix the save's touch goes through its descriptor; the stalled-save test bounds its wait for the truncate (BUG-3838)

Codex r3 Medium: os.Chtimes by path stamps whatever the path names when it
runs, so a path replaced or a symlink retargeted after the open could take
the stamp while the file being truncated kept its old mtime. On Linux, the
BSDs and Darwin the touch is now unix.Futimes on the open descriptor (Linux
resolves it through /proc/self/fd), falling back to the path only when that
fails. TestBUG3838_TouchStampsTheOpenedFile retargets the symlink after the
open and checks which file moved.

Codex r3 Low: the test waited on the seam's truncate signal without a bound;
it now fails if the save returns first or never stalls.

Claude-Session: https://claude.ai/code/session_012QxKpjqXTpU84auY5zZ8FR

* fix(cli): the Unix touch has no path fallback (BUG-3838)

Codex r4 Medium (hunter r4 L2): when the descriptor touch failed, the
os.Chtimes(path) fallback re-resolved the path and could stamp a file other
than the one the save opened. A failed descriptor touch now leaves the save
relying on its truncate, which moves an empty file's mtime on Linux, the one
shipped Unix where futimes can fail for want of /proc.

Claude-Session: https://claude.ai/code/session_012QxKpjqXTpU84auY5zZ8FR

internal/cli/credentials.go

@@ -154,6 +154,11 @@ func loadStoreLocked(path string) (*CredentialStore, error) {
 			return newEmptyStore(), nil
 		}
 		if i >= credentialsEmptyRereads {
+			// A stat that shows a body means a save wrote between the last read
+			// and the stat: read once more rather than answer empty (BUG-3838).
+			if serr == nil && fi.Size() != 0 && i == credentialsEmptyRereads {
+				continue
+			}
 			if serr == nil {
 				emptyConfirmed.set(path, fi)
 			}
@@ -164,10 +169,15 @@ func loadStoreLocked(path string) (*CredentialStore, error) {
 }
 
 // emptyConfirmed is the empty credentials file a locked load already waited
-// out (BUG-3829). A save moves the mtime at its truncate, even of a file that
-// is already empty (measured on ext4, Linux 6.17: ftruncate(0) of an empty
-// file sets mtime to now), so a later empty read of a file a save is
-// rewriting does not match and is re-read again.
+// out (BUG-3829). A save sets the file's mtime to now before it truncates
+// (writeFileInPlace0600), even when the file is already empty, so a later
+// empty read of a file a save is rewriting does not match and is re-read
+// again. The save sets it explicitly because the truncate alone is not
+// enough everywhere: ftruncate(0) of an empty file moved mtime on ext4
+// (Linux 6.17), but POSIX requires that only when the size changes, and
+// macOS and Windows were never measured (BUG-3838). Residuals: a save by a
+// pad built before BUG-3838, or one whose touch fails (a foreign-owned
+// file), relies on the truncate as before.
 //
 // Only a file whose mtime is emptyConfirmedMinAge older than the confirmation
 // is remembered. A later save is stamped no earlier than the confirmation, so
@@ -411,7 +421,13 @@ func writeFileInPlace0600(path string, data []byte) error {
 	}
 	if fi.Mode().IsRegular() {
 		_ = f.Chmod(0600)
-		if err := f.Truncate(0); err != nil {
+		// The mtime is set before the truncate, so a reader's emptyConfirmed
+		// memo stops matching even when the file is already empty: POSIX moves
+		// mtime on ftruncate only when the size changes, and Windows'
+		// SetEndOfFile need not either (BUG-3838). Best effort, like the chmod:
+		// a foreign-owned file we may write but not touch keeps that residual.
+		_ = touchCredentialsFile(f, path)
+		if err := truncateCredentialsFile(f, 0); err != nil {
 			_ = f.Close()
 			return err
 		}
@@ -427,6 +443,12 @@ func writeFileInPlace0600(path string, data []byte) error {
 // statOpenFile is f.Stat, a variable so a test can fail it (BUG-3810).
 var statOpenFile = (*os.File).Stat
 
+// truncateCredentialsFile is f.Truncate, a variable so a test can stall a
+// save between its truncate and its write, and give the truncate the
+// semantics of a platform where it leaves an empty file's mtime alone
+// (BUG-3838).
+var truncateCredentialsFile = (*os.File).Truncate
+
 // credentialsLockWarning is where a write that could not take the lock says
 // so; a variable so a test can read it.
 var credentialsLockWarning io.Writer = os.Stderr

internal/cli/credentials_bug3838_test.go

@@ -0,0 +1,195 @@
+package cli
+
+import (
+	"os"
+	"sync"
+	"testing"
+	"time"
+)
+
+// BUG-3838: the emptyConfirmed memo relied on a save's truncate moving the
+// file's mtime even when the file was already empty. That was measured on
+// Linux only; POSIX moves it only when the size changes. The save now sets
+// the mtime itself before it truncates.
+
+// posixTruncate gives the save's truncate the semantics of a platform where
+// ftruncate leaves an empty file's mtime alone, and stalls the save after it
+// until release is called. truncated is closed once the truncate has run.
+// Cleanup releases a save the test left stalled, so a failing test leaves no
+// goroutine blocked in the seam (codex r1 on BUG-3838).
+func posixTruncate(t *testing.T, path string) (truncated chan struct{}, release func()) {
+	truncated = make(chan struct{})
+	stall := make(chan struct{})
+	var once sync.Once
+	release = func() { once.Do(func() { close(stall) }) }
+	orig := truncateCredentialsFile
+	truncateCredentialsFile = func(f *os.File, size int64) error {
+		before, err := os.Stat(path)
+		if err != nil {
+			return err
+		}
+		if err := orig(f, size); err != nil {
+			return err
+		}
+		if before.Size() == size {
+			if err := os.Chtimes(path, before.ModTime(), before.ModTime()); err != nil {
+				return err
+			}
+		}
+		close(truncated)
+		<-stall
+		return nil
+	}
+	t.Cleanup(func() { truncateCredentialsFile = orig })
+	t.Cleanup(release)
+	return truncated, release
+}
+
+// A process has remembered a genuinely empty file. Another process saves over
+// it without the lock, on a platform whose truncate leaves the mtime alone. A
+// locked load between that save's truncate and its write must not take the
+// memo: it re-reads and loads what the save wrote.
+func TestBUG3838_MemoDoesNotSkipASaveWhoseTruncateKeepsTheMtime(t *testing.T) {
+	home := withTempHome(t)
+	path := writeCredsFile(t, home, "")
+	resetEmptyConfirmed(t)
+	shortRereads(t, 3, time.Millisecond)
+	aged := time.Now().Add(-time.Hour)
+	if err := os.Chtimes(path, aged, aged); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := LoadStore(); err != nil {
+		t.Fatal(err)
+	}
+	emptyConfirmed.mu.Lock()
+	remembered := emptyConfirmed.fi != nil
+	emptyConfirmed.mu.Unlock()
+	if !remembered {
+		t.Fatal("the aged empty file was not remembered; the test needs the memo set")
+	}
+
+	truncated, release := posixTruncate(t, path)
+	saved := make(chan error, 1)
+	go func() { saved <- writeFileInPlace0600(path, []byte(twoServers)) }()
+	// Registered after posixTruncate's, so it runs first: release, then wait.
+	t.Cleanup(func() {
+		release()
+		if err := <-saved; err != nil {
+			t.Error(err)
+		}
+	})
+	select {
+	case <-truncated:
+	case err := <-saved:
+		saved <- err // for the cleanup's receive
+		t.Fatalf("the save returned before its truncate stalled: %v", err)
+	case <-time.After(5 * time.Second):
+		t.Fatal("the save never reached its truncate")
+	}
+	reads := countReads(t, func(n int) {
+		if n == 2 {
+			release()
+			// Wait for the write without consuming the save's result, bounded
+			// so a save that failed after the release fails the test instead
+			// of hanging it (hunter r2 on BUG-3838); cleanup reports its error.
+			for deadline := time.Now().Add(5 * time.Second); time.Now().Before(deadline); {
+				if fi, err := os.Stat(path); err == nil && fi.Size() != 0 {
+					return
+				}
+				time.Sleep(time.Millisecond)
+			}
+			t.Error("the released save never wrote the file")
+		}
+	})
+
+	store, err := loadStoreLocked(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if n := *reads; n < 2 {
+		t.Fatalf("reads = %d: the load answered from the memo during the save", n)
+	}
+	if store.Get("http://a:7777") == nil || store.Get("http://b:7777") == nil {
+		t.Errorf("store = %+v, want the save's two entries", store.Credentials)
+	}
+}
+
+// The last re-read is empty, and a save writes before the stat that follows
+// it: the stat's non-zero size earns one more read instead of an empty store.
+func TestBUG3838_FinalStatShowingABodyRereadsOnce(t *testing.T) {
+	home := withTempHome(t)
+	path := writeCredsFile(t, home, "")
+	resetEmptyConfirmed(t)
+	shortRereads(t, 3, time.Millisecond)
+	orig := readCredentialsFile
+	n := 0
+	readCredentialsFile = func(p string) ([]byte, error) {
+		n++
+		data, err := orig(p)
+		if n == 4 {
+			if werr := os.WriteFile(path, []byte(twoServers), 0600); werr != nil {
+				t.Error(werr)
+			}
+		}
+		return data, err
+	}
+	t.Cleanup(func() { readCredentialsFile = orig })
+
+	store, err := loadStoreLocked(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if n != 5 {
+		t.Errorf("reads = %d, want 5 (1+3 re-reads, then the one the stat earned)", n)
+	}
+	if store.Get("http://a:7777") == nil || store.Get("http://b:7777") == nil {
+		t.Errorf("store = %+v, want the save's two entries", store.Credentials)
+	}
+}
+
+// The touch lands on the file the save opened, not on whatever the path names
+// by the time it runs: a symlink retargeted after the open must not take the
+// stamp while the opened file keeps its old mtime (codex r3 on BUG-3838).
+func TestBUG3838_TouchStampsTheOpenedFile(t *testing.T) {
+	dir := t.TempDir()
+	opened, other, link := dir+"/opened", dir+"/other", dir+"/credentials.json"
+	aged := time.Now().Add(-time.Hour).Truncate(time.Second)
+	for _, p := range []string{opened, other} {
+		if err := os.WriteFile(p, nil, 0600); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.Chtimes(p, aged, aged); err != nil {
+			t.Fatal(err)
+		}
+	}
+	if err := os.Symlink(opened, link); err != nil {
+		t.Skipf("no symlinks here: %v", err)
+	}
+	f, err := os.OpenFile(link, os.O_WRONLY, 0600)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer f.Close()
+	if err := os.Remove(link); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Symlink(other, link); err != nil {
+		t.Fatal(err)
+	}
+
+	if err := touchCredentialsFile(f, link); err != nil {
+		t.Fatalf("touch: %v", err)
+	}
+	for _, c := range []struct {
+		path  string
+		moved bool
+	}{{opened, true}, {other, false}} {
+		fi, err := os.Stat(c.path)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if moved := !fi.ModTime().Equal(aged); moved != c.moved {
+			t.Errorf("%s mtime moved = %v, want %v", c.path, moved, c.moved)
+		}
+	}
+}

internal/cli/credentials_touch_other.go

@@ -0,0 +1,15 @@
+//go:build !windows && !(linux || darwin || freebsd || netbsd || openbsd || dragonfly)
+
+package cli
+
+import (
+	"os"
+	"time"
+)
+
+// touchCredentialsFile sets the save's file's mtime to now (BUG-3838), by
+// path: this platform has no descriptor form wired here.
+func touchCredentialsFile(_ *os.File, path string) error {
+	now := time.Now()
+	return os.Chtimes(path, now, now)
+}

internal/cli/credentials_touch_unix.go

@@ -0,0 +1,30 @@
+//go:build linux || darwin || freebsd || netbsd || openbsd || dragonfly
+
+package cli
+
+import (
+	"os"
+	"time"
+
+	"golang.org/x/sys/unix"
+)
+
+// touchCredentialsFile sets the save's open file's mtime to now (BUG-3838),
+// through its descriptor only, so a path replaced or a symlink retargeted
+// since the open cannot take the stamp instead (codex r3). There is no path
+// fallback (codex r4): when the descriptor form fails (Linux's futimes
+// resolves /proc/self/fd, so no /proc; EPERM on a foreign-owned file) the
+// save relies on its truncate, which moves an empty file's mtime on Linux.
+func touchCredentialsFile(f *os.File, _ string) error {
+	now := time.Now()
+	tv := []unix.Timeval{unix.NsecToTimeval(now.UnixNano()), unix.NsecToTimeval(now.UnixNano())}
+	rc, err := f.SyscallConn()
+	if err != nil {
+		return err
+	}
+	var ferr error
+	if err := rc.Control(func(fd uintptr) { ferr = unix.Futimes(int(fd), tv) }); err != nil {
+		return err
+	}
+	return ferr
+}

internal/cli/credentials_touch_windows.go

@@ -0,0 +1,19 @@
+//go:build windows
+
+package cli
+
+import (
+	"os"
+	"time"
+
+	"golang.org/x/sys/windows"
+)
+
+// touchCredentialsFile sets the save's open file's last-write time to now,
+// through the handle the save already holds (BUG-3838). os.Chtimes would open
+// the path again sharing only FILE_SHARE_WRITE, which fails against a reader
+// holding the file open and, while it is open, makes a reader's open fail.
+func touchCredentialsFile(f *os.File, _ string) error {
+	now := windows.NsecToFiletime(time.Now().UnixNano())
+	return windows.SetFileTime(windows.Handle(f.Fd()), nil, &now, &now)
+}