Anonymous: Add configurable device limit

[akshayutture-augment]

Test 1

---

Replicated from ai-code-review-evaluation/grafana-greptile#1

[greptile-apps]

Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.

[akshayutture-augment]

@greptileai

[greptile-apps]

Greptile Overview

Confidence Score: 1/5

• Critical logic bugs make this PR unsafe to merge
• Two critical bugs will cause incorrect behavior: (1) race condition allows unlimited devices despite limit setting, defeating the feature's purpose, and (2) flawed WHERE clause in updateDevice prevents legitimate updates, causing false "limit reached" errors. Additionally, removed async handling risks authentication timeouts.
• Highest priority: pkg/services/anonymous/anonimpl/anonstore/database.go requires immediate fixes for race condition and update logic. Also review pkg/services/anonymous/anonimpl/client.go for timeout handling.

Important Files Changed

File Analysis

Filename	Score	Overview
pkg/services/anonymous/anonimpl/anonstore/database.go	1/5	Adds device limit enforcement with race condition vulnerability in count-then-insert logic
pkg/services/anonymous/anonimpl/client.go	3/5	Changed from async goroutine to synchronous device tagging, returns ErrDeviceLimitReached to caller
pkg/services/anonymous/anonimpl/impl.go	4/5	Modified constructor to create AnonDBStore with device limit config, propagates errors from tagDeviceUI
pkg/services/anonymous/anonimpl/anonstore/database_test.go	4/5	Added test for device limit enforcement, updated existing tests with new constructor signature

Sequence Diagram

sequenceDiagram
    participant Client
    participant Anonymous as Anonymous Client
    participant AnonDeviceService
    participant AnonDBStore
    participant Database

    Client->>Anonymous: Authenticate(ctx, request)
    Anonymous->>Anonymous: Get org by name
    
    Anonymous->>AnonDeviceService: TagDevice(ctx, httpReq, AnonDeviceUI)
    AnonDeviceService->>AnonDeviceService: Extract device info from request
    AnonDeviceService->>AnonDeviceService: Check local cache
    
    alt Device in cache
        AnonDeviceService-->>Anonymous: Return early (nil)
    else Device not in cache
        AnonDeviceService->>AnonDeviceService: Add to local cache
        AnonDeviceService->>AnonDBStore: CreateOrUpdateDevice(ctx, device)
        
        alt Device limit enabled (> 0)
            AnonDBStore->>Database: CountDevices (30 days window)
            Database-->>AnonDBStore: count
            
            alt count >= deviceLimit
                AnonDBStore->>Database: UPDATE device WHERE device_id = ? AND updated_at BETWEEN ? AND ?
                Database-->>AnonDBStore: rows affected
                
                alt rows affected = 0
                    AnonDBStore-->>AnonDeviceService: ErrDeviceLimitReached
                    AnonDeviceService-->>Anonymous: ErrDeviceLimitReached
                    Anonymous-->>Client: Return error
                end
            else count < deviceLimit
                AnonDBStore->>Database: INSERT ... ON CONFLICT UPDATE
                Database-->>AnonDBStore: Success
            end
        else No limit
            AnonDBStore->>Database: INSERT ... ON CONFLICT UPDATE
            Database-->>AnonDBStore: Success
        end
        
        AnonDeviceService-->>Anonymous: nil
    end
    
    Anonymous->>Anonymous: Create Identity with org info
    Anonymous-->>Client: Return Identity

Loading

[greptile-apps]

_{11 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

logic: Race condition: concurrent requests can bypass limit. Between CountDevices and updateDevice, multiple goroutines can see count < limit and all proceed. Use database-level locking or atomic operations.

Suggested change

	if s.deviceLimit > 0 {
	count, err := s.CountDevices(ctx, time.Now().UTC().Add(-anonymousDeviceExpiration), time.Now().UTC().Add(time.Minute))
	if err != nil {
	return err
	}
	if count >= s.deviceLimit {
	return s.updateDevice(ctx, device)
	}
	// if device limit is reached, only update devices
	if s.deviceLimit > 0 {
	// Try update first - if it succeeds, device exists
	err := s.updateDevice(ctx, device)
	if err == nil {
	return nil
	}
	if !errors.Is(err, ErrDeviceLimitReached) {
	return err
	}
	// Device doesn't exist - check limit with lock or use constraint
	count, err := s.CountDevices(ctx, time.Now().UTC().Add(-anonymousDeviceExpiration), time.Now().UTC().Add(time.Minute))
	if err != nil {
	return err
	}
	if count >= s.deviceLimit {
	return ErrDeviceLimitReached
	}
	}

Prompt To Fix With AI

This is a comment left during a code review.
Path: pkg/services/anonymous/anonimpl/anonstore/database.go
Line: 109:117

Comment:
**logic:** Race condition: concurrent requests can bypass limit. Between `CountDevices` and `updateDevice`, multiple goroutines can see count < limit and all proceed. Use database-level locking or atomic operations.

```suggestion
	// if device limit is reached, only update devices
	if s.deviceLimit > 0 {
		// Try update first - if it succeeds, device exists
		err := s.updateDevice(ctx, device)
		if err == nil {
			return nil
		}
		if !errors.Is(err, ErrDeviceLimitReached) {
			return err
		}
		
		// Device doesn't exist - check limit with lock or use constraint
		count, err := s.CountDevices(ctx, time.Now().UTC().Add(-anonymousDeviceExpiration), time.Now().UTC().Add(time.Minute))
		if err != nil {
			return err
		}
		
		if count >= s.deviceLimit {
			return ErrDeviceLimitReached
		}
	}
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

logic: Flawed logic: updateDevice uses time-based condition that will fail for most update attempts. When device.UpdatedAt is "now" and you check updated_at BETWEEN (now - 30 days) AND (now + 1 min), it only works if DB already has a recent entry. New devices or old devices won't match.

The WHERE clause should only check device_id:

Suggested change

	func (s *AnonDBStore) updateDevice(ctx context.Context, device *Device) error {
	const query = `UPDATE anon_device SET
	client_ip = ?,
	user_agent = ?,
	updated_at = ?
	WHERE device_id = ? AND updated_at BETWEEN ? AND ?`
	args := []interface{}{device.ClientIP, device.UserAgent, device.UpdatedAt.UTC(), device.DeviceID,
	device.UpdatedAt.UTC().Add(-anonymousDeviceExpiration), device.UpdatedAt.UTC().Add(time.Minute),
	const query = `UPDATE anon_device SET
	client_ip = ?,
	user_agent = ?,
	updated_at = ?
	WHERE device_id = ?`
	args := []interface{}{device.ClientIP, device.UserAgent, device.UpdatedAt.UTC(), device.DeviceID}

Prompt To Fix With AI

This is a comment left during a code review.
Path: pkg/services/anonymous/anonimpl/anonstore/database.go
Line: 73:81

Comment:
**logic:** Flawed logic: `updateDevice` uses time-based condition that will fail for most update attempts. When `device.UpdatedAt` is "now" and you check `updated_at BETWEEN (now - 30 days) AND (now + 1 min)`, it only works if DB already has a recent entry. New devices or old devices won't match.

The WHERE clause should only check `device_id`:

```suggestion
	const query = `UPDATE anon_device SET
client_ip = ?,
user_agent = ?,
updated_at = ?
WHERE device_id = ?`

	args := []interface{}{device.ClientIP, device.UserAgent, device.UpdatedAt.UTC(), device.DeviceID}
```

How can I resolve this? If you propose a fix, please make it concise.

[greptile-apps]

style: Changed from async to blocking call without timeout. If TagDevice is slow (DB contention during high traffic), authentication hangs. The old code had a 2-minute timeout.

Prompt To Fix With AI

This is a comment left during a code review.
Path: pkg/services/anonymous/anonimpl/client.go
Line: 44:50

Comment:
**style:** Changed from async to blocking call without timeout. If `TagDevice` is slow (DB contention during high traffic), authentication hangs. The old code had a 2-minute timeout.

How can I resolve this? If you propose a fix, please make it concise.