Grafana / PR #1

Anonymous: Add configurable device limit

grafana/grafana#79265 TypeScript / Go
Golden recall: 100% Extra findings: 5

Golden Comments (4/4 valid, 3 found + 1 partial)

# Comment Severity Status FriendlyReviewer Detail
#1 Race condition: CountDevices and write are not atomic — TOCTOU HIGH ✓ Found FriendlyReviewer described the exact mechanism: multiple concurrent requests can pass the count check and create devices beyond the configured limit (database.go L72)
#2 ErrDeviceLimitReached blocks anonymous authentication entirely (sync vs async before) MEDIUM ✓ Found FriendlyReviewer confirms: tag failures were non-blocking before, now authentication fails completely — severity upgraded to RED (client.go L45)
#3 dbSession.Exec(args...) doesn't compile — signature Exec(string, ...interface{}) vs []interface{} splat MEDIUM N/A (False positive) Benchmark false positive — Grafana's xorm fork has Exec(sqlOrArgs ...any) which accepts the call. The golden set assumed a standard library signature that doesn't match this codebase. Excluded from scoring.
#4 ErrDeviceLimitReached when rowsAffected==0 is misleading — device may not exist LOW ✓ Found FriendlyReviewer described the same bug: "updateDevice returns ErrDeviceLimitReached when rowsAffected==0, but this also occurs for device-not-found or concurrency misses" — severity upgraded to RED (database.go L69)
#5 Time window: device.UpdatedAt used as current time instead of time.Now() for the lower bound LOW ◈ Partial FriendlyReviewer found a related bug: "Upper bound offset of +1 minute in CountDevices may cause minor inconsistency with ListDevices" — not exactly the same issue but in the same code area (database.go L87)

Supplementary Findings (10 findings)

Finding File Severity Legitimate?
updateDevice WHERE clause with tight BETWEEN range can miss existing devices (e.g., old ones), incorrectly returning ErrDeviceLimitReached database.go:97 CRITICAL ✓ Real bug — misses devices with old updated_at
Duplicate constant anonymousDeviceExpiration defined in three files (database.go, api.go, impl.go) database.go:13 MEDIUM ✓ Refactoring justified
TagDevice called synchronously in authentication hot path adds latency (previously async with timeout) client.go:44 MEDIUM ✓ Behavioral change not documented
Cache set before DB write; DB failure silently skips write during cache TTL (29 min) impl.go:81 MEDIUM ✓ Real cache/DB consistency bug
Missing test coverage for device limit path — no tests for limit-hit, limit-zero, existing-device update, or concurrent requests impl_test.go:116 MEDIUM ✓ Coverage gap
Type number | undefined misleading — backend always sends this field (no omitempty) config.ts:200 LOW ✓ TypeScript defect
Default undefined should be 0 to match backend default config.ts:97 LOW ✓ Consistency improvement
ListDevices with partial from/to arguments returns all devices — no error raised database.go:63 LOW ✓ Edge case not handled
ListDevices API does not respect AnonymousDeviceLimit — lists all devices within time window api.go:68 LOW ✓ API inconsistency
Upper bound offset of +1 minute in CountDevices may cause minor inconsistency with ListDevices database.go:87 LOW ✓ Minor temporal inconsistency

Honest Analysis

FriendlyReviewer detected the 3 most critical golden bugs (#1, #2, #4) with severity upgrades across the board — the Medium/Low golden issues were correctly identified as Red/High severity because FriendlyReviewer assessed the runtime impact rather than the original label. Golden #5 was partially covered through a related bug in the same code area (upper bound offset inconsistency), but not the exact time window issue (device.UpdatedAt instead of time.Now()). That nuance was missed.

Golden #3 (Exec signature mismatch) is excluded from scoring. It is a benchmark false positive: the golden set was authored against a generic Go standard library assumption (Exec(string, ...interface{})), but Grafana's xorm fork uses Exec(sqlOrArgs ...any) which accepts the splat call. FriendlyReviewer correctly did not flag this because the code compiles fine. This highlights a known limitation of cross-framework benchmark datasets.

Beyond the golden set, FriendlyReviewer identified 10 legitimate supplementary findings covering the full stack: 1 critical (SQL WHERE clause that misses old devices), 4 medium (duplicate constants, synchronous latency in auth hot path, cache-before-DB consistency bug, missing test coverage), and 5 low (TypeScript types, API inconsistencies, temporal boundary issues). The WHERE clause finding (critical) is a subtle SQL logic error that a human reviewer focused on the diff could easily miss — FriendlyReviewer reads the full function context and catches these edge cases systematically.

The cache-before-DB pattern finding (medium) is particularly interesting: FriendlyReviewer identified that caching before a DB write creates a 29-minute window where a failed DB write is invisible. This is not a logic error in the diff itself, but a behavioral consequence of the ordering — exactly the kind of latent issue systematic review catches.

One finding is arguably noise: the duplicate constant finding across three files is real but relatively minor. In a human review this would likely be a single comment. It is included here because FriendlyReviewer's exhaustive approach trades minor redundancy for completeness — an acceptable trade-off when the alternative is missing real bugs.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech
4 high, 4 medium, 5 low

This merge request introduces a configurable device limit for anonymous sessions to prevent excessive device registration. The configuration contract is mostly consistent across Go and TypeScript layers, but the backend implementation contains several critical issues that affect correctness, error handling, and authentication behavior. The most severe issues include a TOCTOU race condition in the device count-and-write operation, misleading error semantics where non-limit failures are reported as limit reached, and a behavioral change that now blocks anonymous authentication synchronously when the device limit is reached (previously non-blocking). Additionally, there are duplicate constant definitions, a cache-before-DB pattern that silently skips DB writes on failure, and insufficient test coverage for the device limit path. The TypeScript side has two minor type and default inconsistencies. These issues need to be addressed to ensure reliable, predictable behavior and to maintain the non-blocking authentication contract.

These might need a close look

  • 🔴 pkg/services/anonymous/anonimpl/anonstore/database.go (L72)
    TOCTOU race condition: CountDevices and the subsequent write are not atomic, allowing concurrent requests to exceed the configured limit. Use a database-level constraint or lock.
  • 🔴 pkg/services/anonymous/anonimpl/anonstore/database.go (L69)
    updateDevice returns ErrDeviceLimitReached when rowsAffected==0, but this also occurs for device-not-found or concurrency misses. Differentiate error causes.
  • 🔴 pkg/services/anonymous/anonimpl/anonstore/database.go (L97)
    updateDevice WHERE clause with tight time range can miss existing devices (e.g., old ones), incorrectly returning ErrDeviceLimitReached. Widen the condition or query by device_id alone.
  • 🔴 pkg/services/anonymous/anonimpl/client.go (L45)
    ErrDeviceLimitReached now blocks anonymous authentication entirely. Previously, tag failures were non-blocking. Consider logging the error instead of denying access.

Worth checking

  • 🟡 pkg/services/anonymous/anonimpl/anonstore/database.go (L13)
    Duplicate constant anonymousDeviceExpiration defined in three files (database.go, api.go, impl.go). Consolidate into a single location.
  • 🟡 pkg/services/anonymous/anonimpl/client.go (L44)
    TagDevice called synchronously in the authentication hot path adds latency. Previously it was async with a timeout. Evaluate performance impact.
  • 🟡 pkg/services/anonymous/anonimpl/impl.go (L81)
    Cache is set before DB write; failure skips DB write silently for cache TTL (29 min). Cache only on successful write.
  • 🟡 pkg/services/anonymous/anonimpl/impl_test.go (L116)
    Missing test cases for device limit scenario in impl_test.go and database_test.go. Add tests for limit-hit, limit-zero, existing-device update, and concurrent requests.

Small things (take or leave)

  • 🔵 packages/grafana-data/src/types/config.ts (L200)
    Type number | undefined is misleading because the backend always sends this field (no omitempty). Change to plain number.
  • 🔵 packages/grafana-runtime/src/config.ts (L97)
    Default undefined should be 0 to match backend default. Other numeric fields use explicit defaults.
  • 🔵 pkg/services/anonymous/anonimpl/anonstore/database.go (L63)
    ListDevices with partial from/to arguments returns all devices. Consider returning an error or handling partial arguments.
  • 🔵 pkg/services/anonymous/anonimpl/api/api.go (L68)
    ListDevices API does not respect AnonymousDeviceLimit; it lists all devices within time window. Consider filtering or documenting.
  • 🔵 pkg/services/anonymous/anonimpl/anonstore/database.go (L87)
    Upper bound offset of +1 minute in CountDevices may cause minor inconsistency with ListDevices.