askcli: serialize task alias cache writes to fix concurrent rename race

Two concurrent 'ask' invocations could race on the alias cache file:
both wrote the JSON to a shared '<path>.tmp' filename and then both
called os.Rename, so the loser failed with:

  replace task alias cache: rename .../task-aliases-v2.json.tmp
    .../task-aliases-v2.json: no such file or directory

The shared tempfile also enabled lost updates because each process
loaded the file independently before saving its own version on top.

Fix:
- Take an exclusive flock on a sentinel file (task-aliases-v2.json.lock)
  in the cache directory around the full load/modify/save cycle in both
  ensureTaskAliases and resolveTaskSelectorFromCache, using the existing
  internal/filelock package.
- Switch save() to os.CreateTemp so each writer gets a unique tempfile
  name; the loser's tempfile is removed cleanly on rename failure.
- Refactor resolveTaskSelectorFromCache by extracting
  finalizeResolvedTaskSelector to keep functions under 50 lines.

Adds TestEnsureTaskAliases_ConcurrentCallsDoNotRaceOnTempFile, which
reproduces the original error reliably on the unfixed code and now
passes with -race.

Amp-Thread-ID: https://ampcode.com/threads/T-019df49f-52a5-75b1-98d5-371a163ef100
Co-authored-by: Amp <amp@ampcode.com>

3 files changed, 212 insertions, 52 deletions

diff --git a/internal/askcli/task_alias_cache.go b/internal/askcli/task_alias_cache.go
index e89dbb5..8e3fcc4 100644
--- a/internal/askcli/task_alias_cache.go
+++ b/internal/askcli/task_alias_cache.go
@@ -1,17 +1,24 @@
 package askcli
 
 import (
+	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
 	"slices"
 	"time"
 
+	"codeberg.org/snonux/hexai/internal/filelock"
 	"codeberg.org/snonux/hexai/internal/stats"
 )
 
-const taskAliasCacheTTL = 120 * 24 * time.Hour
+const (
+	taskAliasCacheTTL          = 120 * 24 * time.Hour
+	taskAliasCacheLockFileName = "task-aliases-v2.json.lock"
+	taskAliasCacheLockTimeout  = 30 * time.Second
+)
 
 var (
 	nowTaskAliasCache  = time.Now
@@ -39,7 +46,20 @@ type taskAliasCacheEntry struct {
 }
 
 func ensureTaskAliases(tasks []TaskExport) (map[string]string, error) {
-	cache, path, err := loadTaskAliasCache()
+	path, err := taskAliasCachePath()
+	if err != nil {
+		return nil, err
+	}
+	// Hold an advisory lock for the full load/modify/save cycle so concurrent
+	// ask invocations cannot race on the cache file (lost-update or
+	// rename-against-missing-tempfile).
+	unlock, err := acquireTaskAliasCacheLock(filepath.Dir(path))
+	if err != nil {
+		return nil, err
+	}
+	defer func() { _ = unlock() }()
+
+	cache, err := loadTaskAliasCacheAt(path)
 	if err != nil {
 		return nil, err
 	}
@@ -76,17 +96,13 @@ func ensureTaskAliasesForUUIDs(uuids []string) (map[string]string, error) {
 	return ensureTaskAliases(tasks)
 }
 
-func loadTaskAliasCache() (taskAliasCache, string, error) {
-	path, err := taskAliasCachePath()
-	if err != nil {
-		return taskAliasCache{}, "", err
-	}
+func loadTaskAliasCacheAt(path string) (taskAliasCache, error) {
 	data, err := os.ReadFile(path)
 	if os.IsNotExist(err) {
-		return taskAliasCache{}, path, nil
+		return taskAliasCache{}, nil
 	}
 	if err != nil {
-		return taskAliasCache{}, "", fmt.Errorf("read task alias cache: %w", err)
+		return taskAliasCache{}, fmt.Errorf("read task alias cache: %w", err)
 	}
 
 	var cache taskAliasCache
@@ -95,15 +111,42 @@ func loadTaskAliasCache() (taskAliasCache, string, error) {
 		// corruption). Discard and start fresh — tasks will get new alias IDs on
 		// the next run, which is preferable to a hard failure.
 		_ = os.Remove(path)
-		return taskAliasCache{}, path, nil
+		return taskAliasCache{}, nil
 	}
 	if err := cache.validate(); err != nil {
-		return taskAliasCache{}, "", fmt.Errorf("validate task alias cache: %w", err)
+		return taskAliasCache{}, fmt.Errorf("validate task alias cache: %w", err)
 	}
 	// Rebuild lookup maps from the deserialized entries so that all subsequent
 	// operations use O(1) map access rather than a linear scan.
 	cache.rebuildMaps()
-	return cache, path, nil
+	return cache, nil
+}
+
+// acquireTaskAliasCacheLock takes an exclusive advisory lock on a sentinel
+// file in dir so that concurrent ask invocations serialize their
+// load/modify/save of the alias cache. The returned closure releases the lock
+// and closes the underlying file.
+func acquireTaskAliasCacheLock(dir string) (func() error, error) {
+	if err := os.MkdirAll(dir, 0o755); err != nil {
+		return nil, fmt.Errorf("create task alias cache dir: %w", err)
+	}
+	lockPath := filepath.Join(dir, taskAliasCacheLockFileName)
+	f, err := os.OpenFile(lockPath, os.O_CREATE|os.O_RDWR, 0o600)
+	if err != nil {
+		return nil, fmt.Errorf("open task alias cache lock: %w", err)
+	}
+	ctx, cancel := context.WithTimeout(context.Background(), taskAliasCacheLockTimeout)
+	release, err := filelock.AcquireExclusive(ctx, f)
+	cancel()
+	if err != nil {
+		_ = f.Close()
+		return nil, fmt.Errorf("lock task alias cache: %w", err)
+	}
+	return func() error {
+		rerr := release()
+		cerr := f.Close()
+		return errors.Join(rerr, cerr)
+	}, nil
 }
 
 func taskAliasCachePath() (string, error) {
@@ -242,7 +285,8 @@ func (c *taskAliasCache) lookupAliasByUUID(uuid string, now time.Time) (string,
 }
 
 func (c *taskAliasCache) save(path string) error {
-	if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
+	dir := filepath.Dir(path)
+	if err := os.MkdirAll(dir, 0o755); err != nil {
 		return fmt.Errorf("create task alias cache dir: %w", err)
 	}
 	data, err := json.MarshalIndent(c, "", "  ")
@@ -250,16 +294,40 @@ func (c *taskAliasCache) save(path string) error {
 		return fmt.Errorf("marshal task alias cache: %w", err)
 	}
 
-	tempPath := path + ".tmp"
-	if err := os.WriteFile(tempPath, data, 0o600); err != nil {
-		return fmt.Errorf("write task alias cache: %w", err)
+	// Use a unique temp filename per invocation so two concurrent saves cannot
+	// clobber each other's tempfile and produce a "rename: no such file or
+	// directory" error on the loser.
+	tempFile, err := os.CreateTemp(dir, filepath.Base(path)+".*.tmp")
+	if err != nil {
+		return fmt.Errorf("create task alias cache temp: %w", err)
+	}
+	tempPath := tempFile.Name()
+	if err := writeAndCloseTaskAliasTemp(tempFile, tempPath, data); err != nil {
+		_ = os.Remove(tempPath)
+		return err
 	}
 	if err := os.Rename(tempPath, path); err != nil {
+		_ = os.Remove(tempPath)
 		return fmt.Errorf("replace task alias cache: %w", err)
 	}
 	return nil
 }
 
+func writeAndCloseTaskAliasTemp(f *os.File, path string, data []byte) error {
+	if _, err := f.Write(data); err != nil {
+		_ = f.Close()
+		return fmt.Errorf("write task alias cache: %w", err)
+	}
+	if err := f.Chmod(0o600); err != nil {
+		_ = f.Close()
+		return fmt.Errorf("chmod task alias cache: %w", err)
+	}
+	if err := f.Close(); err != nil {
+		return fmt.Errorf("close task alias cache temp %s: %w", path, err)
+	}
+	return nil
+}
+
 func (c *taskAliasCache) sortEntries() {
 	slices.SortFunc(c.Entries, func(a, b taskAliasCacheEntry) int {
 		switch {
diff --git a/internal/askcli/task_alias_cache_test.go b/internal/askcli/task_alias_cache_test.go
index f93a762..7cfa2a7 100644
--- a/internal/askcli/task_alias_cache_test.go
+++ b/internal/askcli/task_alias_cache_test.go
@@ -2,8 +2,10 @@ package askcli
 
 import (
 	"encoding/json"
+	"fmt"
 	"os"
 	"path/filepath"
+	"sync"
 	"testing"
 	"time"
 )
@@ -292,6 +294,87 @@ func TestEnsureTaskAliases_RejectsNextIDReuse(t *testing.T) {
 	}
 }
 
+func TestEnsureTaskAliases_ConcurrentCallsDoNotRaceOnTempFile(t *testing.T) {
+	dir := t.TempDir()
+
+	oldNow := nowTaskAliasCache
+	oldRoot := taskAliasCacheRoot
+	nowTaskAliasCache = func() time.Time { return time.Date(2026, 5, 1, 12, 0, 0, 0, time.UTC) }
+	taskAliasCacheRoot = func() (string, error) { return filepath.Join(dir, "hexai"), nil }
+	defer func() {
+		nowTaskAliasCache = oldNow
+		taskAliasCacheRoot = oldRoot
+	}()
+
+	const goroutines = 16
+	uuids := make([]string, goroutines)
+	for i := range uuids {
+		uuids[i] = fmt.Sprintf("uuid-%02d", i)
+	}
+
+	// Release all goroutines simultaneously to maximise the chance of a race
+	// before the fix (shared .tmp filename + concurrent rename) and to prove
+	// the fix's locking serialises load/modify/save correctly.
+	var start sync.WaitGroup
+	start.Add(1)
+	var done sync.WaitGroup
+	errs := make([]error, goroutines)
+	for i := 0; i < goroutines; i++ {
+		done.Add(1)
+		go func(idx int) {
+			defer done.Done()
+			start.Wait()
+			_, err := ensureTaskAliases([]TaskExport{{UUID: uuids[idx]}})
+			errs[idx] = err
+		}(i)
+	}
+	start.Done()
+	done.Wait()
+
+	for i, err := range errs {
+		if err != nil {
+			t.Fatalf("goroutine %d: ensureTaskAliases returned error: %v", i, err)
+		}
+	}
+
+	path, err := taskAliasCachePath()
+	if err != nil {
+		t.Fatalf("taskAliasCachePath: %v", err)
+	}
+	cache := readTaskAliasCacheForTest(t, path)
+	if got, want := len(cache.Entries), goroutines; got != want {
+		t.Fatalf("len(Entries) = %d, want %d (cache lost updates under concurrency)", got, want)
+	}
+	if got, want := cache.NextID, uint64(goroutines); got != want {
+		t.Fatalf("NextID = %d, want %d", got, want)
+	}
+	for _, uuid := range uuids {
+		if !hasTaskAliasEntry(cache, uuid) {
+			t.Fatalf("expected entry for %s after concurrent writes", uuid)
+		}
+	}
+	// Aliases must be unique — if save() is racy, two UUIDs could share an
+	// alias because both processes saw the same NextID.
+	seen := make(map[string]string, len(cache.Entries))
+	for _, entry := range cache.Entries {
+		if existing, ok := seen[entry.Alias]; ok {
+			t.Fatalf("alias %q assigned to both %s and %s", entry.Alias, existing, entry.UUID)
+		}
+		seen[entry.Alias] = entry.UUID
+	}
+
+	// No leftover .tmp files should remain in the cache directory.
+	entries, err := os.ReadDir(filepath.Dir(path))
+	if err != nil {
+		t.Fatalf("ReadDir: %v", err)
+	}
+	for _, e := range entries {
+		if filepath.Ext(e.Name()) == ".tmp" {
+			t.Fatalf("leftover temp file: %s", e.Name())
+		}
+	}
+}
+
 func readTaskAliasCacheForTest(t *testing.T, path string) taskAliasCache {
 	t.Helper()
 
diff --git a/internal/askcli/task_selector.go b/internal/askcli/task_selector.go
index 033781c..e920192 100644
--- a/internal/askcli/task_selector.go
+++ b/internal/askcli/task_selector.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"path/filepath"
 	"strings"
 )
 
@@ -57,7 +58,17 @@ func resolveTaskSelectorFromCache(selector string, allowAlias bool) (resolvedTas
 		return resolved, nil
 	}
 
-	cache, path, err := loadTaskAliasCache()
+	path, err := taskAliasCachePath()
+	if err != nil {
+		return resolvedTaskSelector{}, err
+	}
+	unlock, err := acquireTaskAliasCacheLock(filepath.Dir(path))
+	if err != nil {
+		return resolvedTaskSelector{}, err
+	}
+	defer func() { _ = unlock() }()
+
+	cache, err := loadTaskAliasCacheAt(path)
 	if err != nil {
 		return resolvedTaskSelector{}, err
 	}
@@ -65,52 +76,50 @@ func resolveTaskSelectorFromCache(selector string, allowAlias bool) (resolvedTas
 	now := nowTaskAliasCache().UTC()
 	changed := cache.prune(now)
 	uuidFromAlias, aliasFound, aliasChanged := cache.lookupUUIDByAlias(selector, now)
-	changed = changed || aliasChanged
 	aliasForUUID, uuidFound, uuidChanged := cache.lookupAliasByUUID(selector, now)
-	changed = changed || uuidChanged
+	changed = changed || aliasChanged || uuidChanged
+
+	return finalizeResolvedTaskSelector(&cache, path, selector,
+		uuidFromAlias, aliasForUUID, aliasFound, uuidFound, changed)
+}
 
+// finalizeResolvedTaskSelector applies the lookup results, persisting any
+// LastAccessedAt updates first when changed is true.
+func finalizeResolvedTaskSelector(
+	cache *taskAliasCache,
+	path, selector, uuidFromAlias, aliasForUUID string,
+	aliasFound, uuidFound, changed bool,
+) (resolvedTaskSelector, error) {
+	saveIfChanged := func() error {
+		if !changed {
+			return nil
+		}
+		return cache.save(path)
+	}
 	switch {
 	case aliasFound && uuidFound && uuidFromAlias != selector:
-		if changed {
-			if err := cache.save(path); err != nil {
-				return resolvedTaskSelector{}, err
-			}
+		if err := saveIfChanged(); err != nil {
+			return resolvedTaskSelector{}, err
 		}
 		return resolvedTaskSelector{}, fmt.Errorf("task selector %q is ambiguous: it matches alias for %s and UUID %s; use uuid:%s to force UUID", selector, uuidFromAlias, selector, selector)
 	case aliasFound:
-		if changed {
-			if err := cache.save(path); err != nil {
-				return resolvedTaskSelector{}, err
-			}
+		if err := saveIfChanged(); err != nil {
+			return resolvedTaskSelector{}, err
 		}
-		return resolvedTaskSelector{
-			Input:     selector,
-			UUID:      uuidFromAlias,
-			Alias:     selector,
-			UsedAlias: true,
-		}, nil
+		return resolvedTaskSelector{Input: selector, UUID: uuidFromAlias, Alias: selector, UsedAlias: true}, nil
 	case uuidFound:
-		if changed {
-			if err := cache.save(path); err != nil {
-				return resolvedTaskSelector{}, err
-			}
-		}
-		return resolvedTaskSelector{
-			Input: selector,
-			UUID:  selector,
-			Alias: aliasForUUID,
-		}, nil
-	default:
-		if IsNumericID(selector) {
-			return resolvedTaskSelector{}, fmt.Errorf(strings.TrimSpace(RejectNumericID()))
+		if err := saveIfChanged(); err != nil {
+			return resolvedTaskSelector{}, err
 		}
-		if changed {
-			if err := cache.save(path); err != nil {
-				return resolvedTaskSelector{}, err
-			}
-		}
-		return resolvedTaskSelector{}, fmt.Errorf("task selector %q did not match a known alias; use uuid:%s to force UUID", selector, selector)
+		return resolvedTaskSelector{Input: selector, UUID: selector, Alias: aliasForUUID}, nil
+	}
+	if IsNumericID(selector) {
+		return resolvedTaskSelector{}, fmt.Errorf(strings.TrimSpace(RejectNumericID()))
+	}
+	if err := saveIfChanged(); err != nil {
+		return resolvedTaskSelector{}, err
 	}
+	return resolvedTaskSelector{}, fmt.Errorf("task selector %q did not match a known alias; use uuid:%s to force UUID", selector, selector)
 }
 
 func looksLikeTaskAlias(selector string) bool {