Fix race condition in one-time software installer download token - #49569
Conversation
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.
4781cd5 to
3334956
Compare
Walkthrough
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/service/redis_lock/redis_lock.go (1)
124-138: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse
GETDELor cache the Lua script
GETDELcovers this atomic read-and-delete path on Redis 6.2+, andredigo.NewScriptavoids resending the script body if older Redis support is still needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/service/redis_lock/redis_lock.go` around lines 124 - 138, Update the atomic read-and-delete logic in the redis lock method using Redis GETDEL, or use a cached redigo.NewScript when older Redis versions must remain supported. Preserve the existing nil-result handling and ctxerr.Wrap error path, and avoid sending the Lua script body on every invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/service/redis_lock/redis_lock_test.go`:
- Around line 144-159: Update the worker function in the concurrent GetAndDelete
test to avoid calling require.NoError from goroutines. Replace it with
assert.NoError, or collect and report errors after wg.Wait from the main test
goroutine, while preserving the winners counting and synchronization behavior.
---
Nitpick comments:
In `@server/service/redis_lock/redis_lock.go`:
- Around line 124-138: Update the atomic read-and-delete logic in the redis lock
method using Redis GETDEL, or use a cached redigo.NewScript when older Redis
versions must remain supported. Preserve the existing nil-result handling and
ctxerr.Wrap error path, and avoid sending the Lua script body on every
invocation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8428d471-4e48-4a85-91c4-0b77fd11b2c4
📒 Files selected for processing (3)
changes/16788-race-condition-one-time-installer-tokenserver/service/redis_lock/redis_lock.goserver/service/redis_lock/redis_lock_test.go
| 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() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does the Go standard library sync.WaitGrouphave aGo method?
💡 Result:
Yes, the Go standard library sync.WaitGroup has a Go method, which was introduced in Go 1.25 [1][2]. The Go method provides a simplified way to manage goroutines with a WaitGroup [2]. It automatically handles the bookkeeping of the internal task counter by calling Add(1) before starting the goroutine and deferring a call to Done within the goroutine to ensure the counter is decremented when the task completes [1][3][2]. Its signature is: func (wg *WaitGroup) Go(f func) This method simplifies the traditional pattern of manually calling Add(1) before spawning a goroutine and ensuring Done is called when it finishes, thereby reducing the risk of counter mismatches [2].
Citations:
- 1: https://pkg.go.dev/sync
- 2: https://appliedgo.net/spotlight/go-1.25-waitgroup-go/
- 3: https://go.dev/src/sync/waitgroup.go?m=text
🏁 Script executed:
set -euo pipefail
printf '--- go.mod ---\n'
if [ -f go.mod ]; then
cat -n go.mod | sed -n '1,80p'
else
echo 'no go.mod found'
fi
printf '\n--- target file excerpt ---\n'
sed -n '120,180p' server/service/redis_lock/redis_lock_test.goRepository: fleetdm/fleet
Length of output: 242
🏁 Script executed:
set -euo pipefail
echo '--- go.mod ---'
if [ -f go.mod ]; then
sed -n '1,120p' go.mod
else
echo 'no go.mod found'
fi
echo
echo '--- server/service/redis_lock/redis_lock_test.go excerpt ---'
sed -n '120,180p' server/service/redis_lock/redis_lock_test.goRepository: fleetdm/fleet
Length of output: 7021
🌐 Web query:
Go testing.T FailNow from spawned goroutine documentation require.NoError goroutine
💡 Result:
Calling testing.T.FailNow from a spawned goroutine is not supported in Go and can lead to unpredictable behavior, including data races [1][2][3]. Official Go documentation explicitly states that FailNow, as well as related methods like Fatal, Fatalf, Skip, SkipNow, and Skipf, must be called only from the goroutine that is running the test or benchmark function [2][4][5]. The reason for this restriction is that FailNow internally calls runtime.Goexit, which terminates only the current goroutine [2][5]. If called from a spawned goroutine, it will terminate that background goroutine rather than the test itself, meaning the test will not stop as intended [6][7][8]. Furthermore, because these methods interact with the test's internal state (which is not designed for concurrent access from multiple goroutines), calling them concurrently can trigger data races or other undefined behavior [9][3]. Regarding testify's require.NoError: Since require.NoError calls FailNow internally, it inherits the same restriction [9][10]. It is not safe to use require functions inside spawned goroutines [9][11]. If you need to perform checks inside a spawned goroutine, you should use the assert package instead (which calls t.Errorf rather than t.FailNow) or return errors through a channel to the main test goroutine, where you can then safely perform fatal assertions [11][8].
Citations:
- 1: testing: detect t.FailNow() called from wrong goroutine golang/go#24678
- 2: https://pkg.go.dev/testing
- 3: chore: avoid concurrent usage of t.FailNow coder/coder#1683
- 4: https://pkg.go.dev/testing@latest
- 5: https://tip.golang.org/src/testing/testing.go?m=text
- 6: https://www.jetbrains.com/help/inspectopedia/GoVetFailNowInNotTestGoroutine.html
- 7: https://deepsource.com/directory/go/issues/GO-W6004
- 8: fix: remove use of testing.T.FailNow() inside goroutine IBM/sarama#1601
- 9: Document that require is not safe to call in created goroutines stretchr/testify#1392
- 10: https://sierikov.dev/writing/not-all-failures-are-equal
- 11: Wrong assertion on tests kubernetes/kubernetes#130346
Avoid require.NoError inside the worker goroutines. require.NoError calls FailNow, which is only supported from the test goroutine. Use assert.NoError here or report the error back to the main test goroutine.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/service/redis_lock/redis_lock_test.go` around lines 144 - 159, Update
the worker function in the concurrent GetAndDelete test to avoid calling
require.NoError from goroutines. Replace it with assert.NoError, or collect and
report errors after wg.Wait from the main test goroutine, while preserving the
winners counting and synchronization behavior.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #49569 +/- ##
=======================================
Coverage 67.81% 67.82%
=======================================
Files 3890 3890
Lines 247631 247635 +4
Branches 13175 13175
=======================================
+ Hits 167942 167952 +10
+ Misses 64524 64522 -2
+ Partials 15165 15161 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3 participants
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.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary by CodeRabbit