Anonymous: Add configurable device limit
grafana/grafana#79265
Golden recall: 40%
Extra findings: 9
Golden Comments (2/5 found)
| # | Comment | Severity | Status | FriendlyReviewer Detail |
|---|---|---|---|---|
| #1 | Race condition: Multiple concurrent requests could pass the device count check simultaneously and create devices beyond the limit. Consider using a database transaction or lock. | HIGH | ✓ Found | "Count-then-insert in CreateOrUpdateDevice is a TOCTOU race. Concurrent requests with different device IDs can both pass the count check and exceed the limit. Guard with a transaction/lock or DB constraint." (database.go L109) |
| #2 | Anonymous authentication now fails entirely if anonDeviceService.TagDevice returns ErrDeviceLimitReached. Previously, device tagging was asynchronous and non-blocking. This change prevents anonymous users from authenticating when the device limit is reached. |
MEDIUM | ✓ Found | "TagDevice is now called synchronously in Authenticate, putting a COUNT + INSERT/UPDATE on the auth critical path. Confirm the latency regression is intended; consider backgrounding the update for the still-at-limit / existing-device case." (client.go L44) |
| #3 | This call won't compile: dbSession.Exec(args...) is given a []interface{} where the first element is the query, but Exec's signature requires a first parameter of type string (not an interface{} splat). |
MEDIUM | ✗ Not found | No candidate mentions the compile defect: a []interface{} is passed where the first element is the query, but Exec requires the first parameter to be a string. The review covers the cache, the recency window, the TOCTOU race, the wire binding and the frontend contract — not this typing defect. See pkg/services/anonymous/anonimpl/anonstore/database.go:85 |
| #4 | Returning ErrDeviceLimitReached when no rows were updated is misleading; the device might not exist. |
LOW | ✗ Not found | The review discusses RowsAffected()==0 (database.go L80) in updateDevice to detect new devices, not the error type returned when no rows are updated — the device might not exist. See pkg/services/anonymous/anonimpl/anonstore/database.go:96 |
| #5 | Time window calculation inconsistency: Using device.UpdatedAt.UTC().Add(-anonymousDeviceExpiration) as the lower bound but device.UpdatedAt as the current time may not match the intended logic. Consider using time.Now().UTC() consistently. |
LOW | ✗ Not found | The review criticizes the 30-day recency window (Postgres portability, false rejections) but not the temporal reference inconsistency: device.UpdatedAt.UTC().Add(-anonymousDeviceExpiration) is the lower bound while device.UpdatedAt doubles as "now". See pkg/services/anonymous/anonimpl/anonstore/database.go:80-81 |
Supplementary Findings (9 findings)
| Finding | File | Severity | Legitimate? |
|---|---|---|---|
Local cache populated before the DB write — an ErrDeviceLimitReached leaves the key cached ~29 min, so the next request from the same device bypasses the limit |
impl.go:78 | HIGH | ✓ Real limit-evasion path, arguably as severe as the scored race; fixed by populating the cache only after a successful write |
updateDevice's 30-day recency window + RowsAffected()==0 falsely rejects existing-but-stale devices and is not portable (Postgres no-op UPDATEs report 0 rows) |
database.go:80 | HIGH | ✓ Real false-rejection path for legitimate returning devices; a user-facing enforcement flaw |
ProvideAnonDBStore signature changed but wire.go still binds it with an unfillable int64 argument — stale wire binding |
database.go:52 | MEDIUM | ✓ Stale wire binding is a real DI hazard; presumably caught by generation, but the inconsistency stands |
Frontend contract mismatch: the backend always serializes anonymousDeviceLimit (no omitempty), so the TS 'number | undefined' type and the lodash-merge default can never occur |
config.ts:200 | MEDIUM | ✓ Real inconsistency between the TS contract and the Go serialization |
Swagger annotation says /stats while the endpoint is registered under /api/anonymous/devices |
api/api.go:56 | LOW | ✓ Documentation mismatch, cosmetic but real |
AnonymousDeviceLimit parsed with MustInt64(0) without validation — negative values silently behave as unlimited |
setting.go:1654 | LOW | ✓ Genuine config-validation gap: silent unbounded behavior on a misconfigured value |
| Service-level tests never exercise the limit path (default 0), missing the cache-poisoning, TOCTOU, error-return and cleanup paths | impl_test.go:116 | LOW | ✓ Real coverage gap; it plausibly contributed to the storage-layer misses |
30-day window constant duplicated across three files while cleanup uses keepFor (61d) — devices physically present but not counting toward the limit |
database.go:16 | LOW | ✓ Real constant-drift mismatch affecting limit accounting |
| Device limit exposed to anonymous users themselves via the frontend settings endpoint | frontendsettings.go:198 | LOW | ✓ Borderline: mirrors AnonymousEnabled; a disclosure-policy question rather than a bug |
Honest Analysis
FriendlyReviewer's full review
As posted on the PR — the complete output from FriendlyReviewer.
Hi there 👋
| 🌥️ Tech | 🌥️ Feat |
|---|---|
| 3 high, 3 medium, 5 low | 1 ok, 2 issues |
The MR introduces a configurable anonymous device limit across the storage, service, API, and frontend settings layers. The configuration wiring is largely consistent, but the enforcement mechanism has several critical flaws: the local cache can bypass the limit after the first rejection, the update heuristic rejects legitimate returning devices and is not cross-dialect safe, and the count-then-insert flow is racy. In addition, the frontend contract is inconsistent with backend serialization, and there are gaps in test coverage that leave these issues undetected. The feature does not yet reliably meet the user-facing acceptance criteria and should be revised before merge.
These might need a close look
- 🔴 pkg/services/anonymous/anonimpl/impl.go (L78)
The local cache is populated before the DB write. When the store returnsErrDeviceLimitReached, the key remains cached for ~29 minutes, so the next request from the same device returns nil early and bypasses the limit. Only populate the cache after a successful write or delete it on the error path. - 🔴 pkg/services/anonymous/anonimpl/anonstore/database.go (L80)
updateDevice uses a 30-day recency window +RowsAffected()==0to decide whether a device is new. This falsely rejects existing-but-stale devices and is not portable (Postgres no-op UPDATEs report 0 rows). Key the update on device_id existence rather than a recency window, and decide 'limit reached' atomically. - 🔴 pkg/services/anonymous/anonimpl/anonstore/database.go (L109)
Count-then-insert in CreateOrUpdateDevice is a TOCTOU race. Concurrent requests with different device IDs can both pass the count check and exceed the limit. Guard with a transaction/lock or DB constraint.
Worth checking
- 🟡 pkg/services/anonymous/anonimpl/anonstore/database.go (L52)
ProvideAnonDBStore signature changed but pkg/server/wire.go still binds anonstore.ProvideAnonDBStore to AnonStore with an unfillable int64 argument. Remove the stale wire binding and regenerate wire_gen.go. - 🟡 pkg/services/anonymous/anonimpl/client.go (L44)
TagDevice is now called synchronously in Authenticate, putting a COUNT + INSERT/UPDATE on the auth critical path. Confirm the latency regression is intended; consider backgrounding the update for the still-at-limit / existing-device case. - 🟡 packages/grafana-data/src/types/config.ts (L200)
Backend always serializes anonymousDeviceLimit (no omitempty), so the frontend 'number | undefined' / 'undefined' default can never occur; lodash merge always overwrites with 0. Align the contract: either type it as 'number' with default 0, or add omitempty to the DTO.
Small things (take or leave)
- 🔵 pkg/services/anonymous/anonimpl/api/api.go (L56)
Swagger annotation uses /stats but the endpoint is registered under /api/anonymous/devices. Correct the annotation or registration to match. - 🔵 pkg/setting/setting.go (L1654)
AnonymousDeviceLimit is parsed with MustInt64(0) without validation. Negative values silently behave as unlimited (since the store only enforces when deviceLimit > 0). Validate >= 0 or at least log a warning. The same non-guard appears in ProvideAnonDBStore at database.go:109. - 🔵 pkg/services/anonymous/anonimpl/impl_test.go (L116)
Service-level tests never exercise the limit path because AnonymousDeviceLimit defaults to 0. This misses the cache-poisoning-on-failure bug, the return of ErrDeviceLimitReached through TagDevice, the concurrent race, and the cleanup path. database_test.go:51 also only covers rejection of a new device at the limit, not the existing-device update behavior. - 🔵 pkg/services/anonymous/anonimpl/anonstore/database.go (L16)
The 30-day window constant is duplicated in database.go, api/api.go, and impl.go (thirtyDays), while cleanup uses keepFor (61d). The 30d-vs-61d mismatch makes a device physically present but not counting toward the limit. Unify the constant and document the relationship. - 🔵 pkg/api/frontendsettings.go (L198)
The device limit is exposed to anonymous users themselves. This mirrors AnonymousEnabled but is still disclosed to the population being limited. Confirm this is intentional; if admin-only, gate behind c.IsSignedIn.
✅ Feature-level checklist
Looks good
- ✅ If this is a pre-GA feature, it is behind a feature toggle.
No evidence this is a pre-GA feature; the limit is configurable via a setting rather than a feature toggle, which is appropriate for a production configuration knob.
Not there yet
- ❌ It works as expected from a user's perspective.
The limit can be bypassed on the second request from a device (cache poisoning), and legitimate returning devices may be falsely rejected due to the recency-window logic. The enforcement is not reliable. - ❌ The docs are updated.
No documentation updates were observed in the MR. The checklist item is not satisfied as far as the review can determine.
FriendlyReviewer found 2 of the 5 golden comments of this PR — recall 40%. The two found are the most consequential: the HIGH count-then-insert TOCTOU race (database.go L109, 🔴) and the MEDIUM synchronous
TagDevicecall on the auth critical path (client.go L44, 🟡). Both matched directly on root cause and proposed fix — a transaction/lock for the race, backgrounding the update for the latency regression.The three misses are all in the storage layer, and they are telling. Golden #3 is a compile-time defect:
dbSession.Exec(args...)splats a[]interface{}whereExecrequires the first argument to be a string. The review examined database.go extensively — recency window,RowsAffected(), TOCTOU — but approached the file from enforcement-correctness, not API typing and error semantics, which is exactly where goldens #3, #4 and #5 sit: an impossible call signature, a misleading error type when no rows are updated, and an inconsistent temporal reference (UpdatedAtas both lower bound and "now").The nine supplementary findings are all defensible. The strongest is the cache-poisoning finding (impl.go L78): the key is stored before the DB write, so a rejected device bypasses the limit for ~29 minutes — a limit-evasion bug of the same order as the scored race. The stale wire binding (database.go L52) and the frontend contract mismatch (config.ts L200) are real but would surface in a build or integration test. The rest are legitimate but lighter: a swagger annotation, a missing config validation, coverage gaps, a duplicated constant, and a disclosure-policy question.
Overall the review found the two most serious defects on the PR and a broad set of legitimate extras, but it missed three dataset-credited storage-layer issues — including one that would have failed compilation. The net is a strong enforcement-focused review with a blind spot for typing and error-semantics defects in the same file it covered most thoroughly.