Fix race condition in one-time software installer download token

Token redemption consumed the key via a non-atomic read-then-delete,
which allowed concurrent requests to redeem the same one-time token more
than once. Made consumption atomic so a token can only be used once,
even under concurrent access, and added coverage for the concurrent
path.

changes/16788-race-condition-one-time-installer-token

@@ -0,0 +1 @@
+- Fixed a race condition that allowed a one-time software installer download token to be redeemed more than once when many requests raced concurrently, by making the token consumption atomic.

server/service/redis_lock/redis_lock.go

@@ -121,19 +121,21 @@ func (r *redisLock) GetAndDelete(ctx context.Context, key string) (*string, erro
 	conn := redis.ConfigureDoer(r.pool, r.pool.Get())
 	defer conn.Close()
 
-	// Note: In Redis 6.2.0, this can be accomplished with a single command: GETDEL.
+	// GET and DEL must run atomically so a key can be consumed only once.
+	const getDelScript = `
+		local v = redis.call("get", KEYS[1])
+		if v then
+			redis.call("del", KEYS[1])
+		end
+		return v
+	`
 
-	res, err := redigo.String(conn.Do("GET", r.testPrefix+key))
+	res, err := redigo.String(conn.Do("EVAL", getDelScript, 1, r.testPrefix+key))
 	if errors.Is(err, redigo.ErrNil) {
 		return nil, nil
 	}
 	if err != nil {
-		return nil, ctxerr.Wrap(ctx, err, "redis GET")
-	}
-
-	_, err = conn.Do("DEL", r.testPrefix+key)
-	if err != nil {
-		return nil, ctxerr.Wrap(ctx, err, "redis DEL")
+		return nil, ctxerr.Wrap(ctx, err, "redis GET/DEL")
 	}
 
 	return &res, nil

server/service/redis_lock/redis_lock_test.go

@@ -2,6 +2,8 @@ package redis_lock
 
 import (
 	"context"
+	"sync"
+	"sync/atomic"
 	"testing"
 	"time"
 
@@ -16,6 +18,7 @@ func TestRedisLock(t *testing.T) {
 	for _, f := range []func(*testing.T, fleet.Lock){
 		testRedisAcquireLock,
 		testRedisSet,
+		testRedisGetAndDeleteConcurrent,
 	} {
 		t.Run(test.FunctionName(f), func(t *testing.T) {
 			t.Run("standalone", func(t *testing.T) {
@@ -126,6 +129,38 @@ func testRedisAcquireLock(t *testing.T, lock fleet.Lock) {
 	assert.Nil(t, getResult)
 }
 
+// testRedisGetAndDeleteConcurrent asserts that GetAndDelete consumes a key
+// atomically: when many callers race for the same key, exactly one gets the
+// value and all others get nil. This guards the single-use guarantee relied on
+// by one-time software installer download tokens.
+func testRedisGetAndDeleteConcurrent(t *testing.T, lock fleet.Lock) {
+	ctx := context.Background()
+
+	const workers = 50
+	result, err := lock.SetIfNotExist(ctx, "raceKey", "1", 0)
+	require.NoError(t, err)
+	require.True(t, result)
+
+	var wg sync.WaitGroup
+	var start sync.WaitGroup
+	var winners atomic.Int64
+	start.Add(1)
+	for range workers {
+		wg.Go(func() {
+			start.Wait() // release all goroutines at once to maximize contention
+			got, err := lock.GetAndDelete(ctx, "raceKey")
+			require.NoError(t, err)
+			if got != nil {
+				winners.Add(1)
+			}
+		})
+	}
+	start.Done()
+	wg.Wait()
+
+	assert.Equal(t, int64(1), winners.Load(), "exactly one caller should consume the key")
+}
+
 func testRedisSet(t *testing.T, lock fleet.Lock) {
 	ctx := context.Background()