fix(tokenstore): fix race condition and cache sync in revocation
- Rebuild revokedSet from revokedList in Revoke() to keep in sync - Move lookup inside lock in IsRevoked() to prevent race condition - Add test case for verifying old revoked tokens persist
- commit
- c57e4be96fc6e9267b6e35e9fe5eb1531f137db9
- author
- brice · · 2026-01-18 09:52 UTC
- parent
- 21240d8
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
2 files · +14 -5
M internal/adapters/out/tokenstore/pass.go +5 -2
| @@ -242,7 +242,10 @@ func (s *PassStore) Revoke(ctx context.Context, tokenID string) error { | |||
| 242 | 242 | // Update cache | |
| 243 | 243 | s.cacheMu.Lock() | |
| 244 | 244 | s.revokedList = revokedList | |
| 245 | - | s.revokedSet[tokenID] = struct{}{} | |
| 245 | + | s.revokedSet = make(map[string]struct{}, len(revokedList)) | |
| 246 | + | for _, id := range revokedList { | |
| 247 | + | s.revokedSet[id] = struct{}{} | |
| 248 | + | } | |
| 246 | 249 | s.cacheMu.Unlock() | |
| 247 | 250 | ||
| 248 | 251 | s.log.Info(). | |
| @@ -280,9 +283,9 @@ func (s *PassStore) IsRevoked(ctx context.Context, tokenID string) (bool, error) | |||
| 280 | 283 | for _, id := range revokedList { | |
| 281 | 284 | s.revokedSet[id] = struct{}{} | |
| 282 | 285 | } | |
| 286 | + | _, revoked := s.revokedSet[tokenID] | |
| 283 | 287 | s.cacheMu.Unlock() | |
| 284 | 288 | ||
| 285 | - | _, revoked := s.revokedSet[tokenID] | |
| 286 | 289 | return revoked, nil | |
| 287 | 290 | } | |
| 288 | 291 | ||
| @@ -242,7 +242,10 @@ func (s *PassStore) Revoke(ctx context.Context, tokenID string) error { | |||||
| 242 | // Update cache | 242 | // Update cache | ||
| 243 | s.cacheMu.Lock() | 243 | s.cacheMu.Lock() | ||
| 244 | s.revokedList = revokedList | 244 | s.revokedList = revokedList | ||
| 245 | - | s.revokedSet[tokenID] = struct{}{} | 245 | + | s.revokedSet = make(map[string]struct{}, len(revokedList)) |
| 246 | + | for _, id := range revokedList { | |||
| 247 | + | s.revokedSet[id] = struct{}{} | |||
| 248 | + | } | |||
| 246 | s.cacheMu.Unlock() | 249 | s.cacheMu.Unlock() | ||
| 247 | 250 | ||||
| 248 | s.log.Info(). | 251 | s.log.Info(). | ||
| @@ -280,9 +283,9 @@ func (s *PassStore) IsRevoked(ctx context.Context, tokenID string) (bool, error) | |||||
| 280 | for _, id := range revokedList { | 283 | for _, id := range revokedList { | ||
| 281 | s.revokedSet[id] = struct{}{} | 284 | s.revokedSet[id] = struct{}{} | ||
| 282 | } | 285 | } | ||
| 286 | + | _, revoked := s.revokedSet[tokenID] | |||
| 283 | s.cacheMu.Unlock() | 287 | s.cacheMu.Unlock() | ||
| 284 | 288 | ||||
| 285 | - | _, revoked := s.revokedSet[tokenID] | |||
| 286 | return revoked, nil | 289 | return revoked, nil | ||
| 287 | } | 290 | } | ||
| 288 | 291 | ||||
M internal/adapters/out/tokenstore/pass_test.go +9 -3
| @@ -234,9 +234,9 @@ func TestPassStore_IsRevoked_ConcurrentAccess(t *testing.T) { | |||
| 234 | 234 | func TestPassStore_RevokeUpdatesCache(t *testing.T) { | |
| 235 | 235 | store := newTestPassStore() | |
| 236 | 236 | ||
| 237 | - | // Initialize empty revocation cache | |
| 237 | + | // Initialize revocation cache with an older revoked token | |
| 238 | 238 | store.cacheMu.Lock() | |
| 239 | - | store.revokedList = []string{} | |
| 239 | + | store.revokedList = []string{"oldly-revoked"} | |
| 240 | 240 | store.revokedSet = make(map[string]struct{}) | |
| 241 | 241 | store.cacheMu.Unlock() | |
| 242 | 242 | ||
| @@ -244,11 +244,17 @@ func TestPassStore_RevokeUpdatesCache(t *testing.T) { | |||
| 244 | 244 | tokenID := "newly-revoked" | |
| 245 | 245 | store.cacheMu.Lock() | |
| 246 | 246 | store.revokedList = append(store.revokedList, tokenID) | |
| 247 | - | store.revokedSet[tokenID] = struct{}{} | |
| 247 | + | store.revokedSet = make(map[string]struct{}, len(store.revokedList)) | |
| 248 | + | for _, id := range store.revokedList { | |
| 249 | + | store.revokedSet[id] = struct{}{} | |
| 250 | + | } | |
| 248 | 251 | store.cacheMu.Unlock() | |
| 249 | 252 | ||
| 250 | 253 | // Verify IsRevoked now returns true from cache | |
| 251 | 254 | revoked, err := store.IsRevoked(context.Background(), tokenID) | |
| 252 | 255 | require.NoError(t, err) | |
| 253 | 256 | assert.True(t, revoked) | |
| 257 | + | revoked, err = store.IsRevoked(context.Background(), "oldly-revoked") | |
| 258 | + | require.NoError(t, err) | |
| 259 | + | assert.True(t, revoked) | |
| 254 | 260 | } | |
| @@ -234,9 +234,9 @@ func TestPassStore_IsRevoked_ConcurrentAccess(t *testing.T) { | |||||
| 234 | func TestPassStore_RevokeUpdatesCache(t *testing.T) { | 234 | func TestPassStore_RevokeUpdatesCache(t *testing.T) { | ||
| 235 | store := newTestPassStore() | 235 | store := newTestPassStore() | ||
| 236 | 236 | ||||
| 237 | - | // Initialize empty revocation cache | 237 | + | // Initialize revocation cache with an older revoked token |
| 238 | store.cacheMu.Lock() | 238 | store.cacheMu.Lock() | ||
| 239 | - | store.revokedList = []string{} | 239 | + | store.revokedList = []string{"oldly-revoked"} |
| 240 | store.revokedSet = make(map[string]struct{}) | 240 | store.revokedSet = make(map[string]struct{}) | ||
| 241 | store.cacheMu.Unlock() | 241 | store.cacheMu.Unlock() | ||
| 242 | 242 | ||||
| @@ -244,11 +244,17 @@ func TestPassStore_RevokeUpdatesCache(t *testing.T) { | |||||
| 244 | tokenID := "newly-revoked" | 244 | tokenID := "newly-revoked" | ||
| 245 | store.cacheMu.Lock() | 245 | store.cacheMu.Lock() | ||
| 246 | store.revokedList = append(store.revokedList, tokenID) | 246 | store.revokedList = append(store.revokedList, tokenID) | ||
| 247 | - | store.revokedSet[tokenID] = struct{}{} | 247 | + | store.revokedSet = make(map[string]struct{}, len(store.revokedList)) |
| 248 | + | for _, id := range store.revokedList { | |||
| 249 | + | store.revokedSet[id] = struct{}{} | |||
| 250 | + | } | |||
| 248 | store.cacheMu.Unlock() | 251 | store.cacheMu.Unlock() | ||
| 249 | 252 | ||||
| 250 | // Verify IsRevoked now returns true from cache | 253 | // Verify IsRevoked now returns true from cache | ||
| 251 | revoked, err := store.IsRevoked(context.Background(), tokenID) | 254 | revoked, err := store.IsRevoked(context.Background(), tokenID) | ||
| 252 | require.NoError(t, err) | 255 | require.NoError(t, err) | ||
| 253 | assert.True(t, revoked) | 256 | assert.True(t, revoked) | ||
| 257 | + | revoked, err = store.IsRevoked(context.Background(), "oldly-revoked") | |||
| 258 | + | require.NoError(t, err) | |||
| 259 | + | assert.True(t, revoked) | |||
| 254 | } | 260 | } | ||