feat: ensure reliable concurrent token file access with file locking #18
Conversation
appleboy
commented
Jan 22, 2026
appleboy
commented
- Add file-based locking to prevent concurrent access to the token file
- Refactor flag initialization to avoid conflicts with test flag parsing
- Ensure atomic writes to the token file using a temp file and rename
- Acquire and release file locks during token save operations for consistency
- Add comprehensive tests and benchmarks for file locking and concurrent token file access
- Improve reliability of concurrent token storage and preserve tokens for multiple clients
- Add file-based locking to prevent concurrent access to the token file - Refactor flag initialization to avoid conflicts with test flag parsing - Ensure atomic writes to the token file using a temp file and rename - Acquire and release file locks during token save operations for consistency - Add comprehensive tests and benchmarks for file locking and concurrent token file access - Improve reliability of concurrent token storage and preserve tokens for multiple clients Signed-off-by: Bo-Yi Wu <appleboy.tw@gmail.com>
✅ Deploy Preview for authgate-demo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
This PR adds reliable concurrent token file access through file-based locking, preventing race conditions when multiple processes or goroutines access the token file simultaneously. The implementation uses an atomic write pattern with temp files and file locks, along with comprehensive tests and benchmarks.
Changes:
- Implemented file-based locking mechanism with stale lock cleanup and retry logic
- Refactored flag initialization to avoid conflicts with test flag parsing by moving
flag.Parse()frominit()to a separateinitConfig()function - Enhanced token save operation with atomic writes using temp file + rename pattern and file locking for consistency
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| _example/authgate-cli/filelock.go | New file implementing file lock acquisition/release with retry logic and stale lock cleanup |
| _example/authgate-cli/filelock_test.go | Comprehensive tests for file locking including concurrent access, stale locks, timeouts, and benchmarks |
| _example/authgate-cli/main.go | Refactored flag initialization and enhanced saveTokens with file locking and atomic writes |
| _example/authgate-cli/main_test.go | Tests for concurrent token saves, client preservation, and performance benchmarks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err := lock.release(); err == nil { | ||
| t.Logf("Second release returned: %v (expected error)", err) |
There was a problem hiding this comment.
The test logic for verifying multiple releases is inverted. The comment on line 222 states "Second release should not panic (idempotent)" but the test on line 223 checks if err == nil and logs "expected error". If the second release is expected to be idempotent and not error, the test should check for err == nil as a success condition, not log it as unexpected. Alternatively, if an error is expected, the comment should reflect that.
| if err := lock.release(); err == nil { | |
| t.Logf("Second release returned: %v (expected error)", err) | |
| if err := lock.release(); err != nil { | |
| t.Errorf("Second release failed: %v", err) |
| if err != nil { | ||
| return fmt.Errorf("failed to acquire lock: %w", err) | ||
| } | ||
| defer lock.release() |
There was a problem hiding this comment.
The lock.release() error is ignored in the defer statement. If the lock cannot be released, this could lead to leaked lock files and potential deadlocks for subsequent operations. Consider capturing and logging the error, or returning it through a named return value.
| tempDir := t.TempDir() | ||
| tokenFile = filepath.Join(tempDir, "tokens.json") |
There was a problem hiding this comment.
The test modifies the global tokenFile variable on line 28, which could cause issues if multiple tests run concurrently. While this test itself uses proper synchronization internally, it doesn't protect against interference from other tests that might also modify this global variable. Consider using test-specific parameters or ensuring tests don't run in parallel.
| tempDir := t.TempDir() | |
| tokenFile = filepath.Join(tempDir, "tokens.json") | |
| tempDir := t.TempDir() | |
| originalTokenFile := tokenFile | |
| tokenFile = filepath.Join(tempDir, "tokens.json") | |
| defer func() { | |
| tokenFile = originalTokenFile | |
| }() |
| clientID = "bench-client" | ||
|
|
||
| storage := &TokenStorage{ | ||
| AccessToken: "access-token", | ||
| RefreshToken: "refresh-token", | ||
| TokenType: "Bearer", | ||
| ExpiresAt: time.Now().Add(1 * time.Hour), | ||
| ClientID: clientID, |
There was a problem hiding this comment.
The benchmark modifies the global tokenFile and clientID variables, which can cause race conditions when benchmarks run in parallel or alongside tests. This is particularly problematic since this benchmark uses b.RunParallel which creates concurrent execution. Consider using benchmark-specific instances to avoid global state mutation.
| clientID = "bench-client" | |
| storage := &TokenStorage{ | |
| AccessToken: "access-token", | |
| RefreshToken: "refresh-token", | |
| TokenType: "Bearer", | |
| ExpiresAt: time.Now().Add(1 * time.Hour), | |
| ClientID: clientID, | |
| benchClientID := "bench-client" | |
| storage := &TokenStorage{ | |
| AccessToken: "access-token", | |
| RefreshToken: "refresh-token", | |
| TokenType: "Bearer", | |
| ExpiresAt: time.Now().Add(1 * time.Hour), | |
| ClientID: benchClientID, |
| func (fl *fileLock) release() error { | ||
| if fl.lockFile != nil { | ||
| fl.lockFile.Close() | ||
| } | ||
| return os.Remove(fl.lockPath) |
There was a problem hiding this comment.
The error from os.Remove is ignored when releasing the lock file. If the lock file cannot be removed, this error should be checked and handled appropriately. Consider returning the error from lockFile.Close() as well, as closing the file could fail.
| clientID = "client-1" | ||
| storage1 := &TokenStorage{ | ||
| AccessToken: "token-1", | ||
| RefreshToken: "refresh-1", | ||
| TokenType: "Bearer", | ||
| ExpiresAt: time.Now().Add(1 * time.Hour), | ||
| ClientID: "client-1", | ||
| } | ||
| if err := saveTokens(storage1); err != nil { | ||
| t.Fatalf("Failed to save first client: %v", err) | ||
| } | ||
|
|
||
| // Save second client (should preserve first) | ||
| clientID = "client-2" |
There was a problem hiding this comment.
Modifying the global tokenFile and clientID variables within tests creates potential race conditions when tests run in parallel using t.Parallel() or when running with -race flag. This can cause tests to interfere with each other and produce flaky results. Consider using test-specific instances or dependency injection to avoid modifying global state.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 participant
