From 90feabaf14b67ef61124881164ef3636b1827398 Mon Sep 17 00:00:00 2001
From: Paul Buetow <paul@buetow.org>
Date: Mon, 4 May 2026 23:20:21 +0300
Subject: 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>
---
 internal/askcli/task_selector.go | 81 ++++++++++++++++++++++------------------
 1 file changed, 45 insertions(+), 36 deletions(-)

(limited to 'internal/askcli/task_selector.go')

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 {
-- 
cgit v1.2.3