refactor: usagelimiter with redis #3800
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughAdds a Redis-backed usage limiter (counter-based atomic decrements, async DB replay, metrics, Close), wires it into keys service and test harness, extends Counter API and Redis implementation, adds multi-node integration tests and a test generator, and small related API, buffer, and ratelimit adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant LB as Load Balancer
participant API as API Node
participant Keys as Keys Service
participant UL as Usage Limiter (Redis)
participant RC as Redis Counter
participant DB as Database
Client->>LB: POST /v2/keys.verifyKey (cost)
LB->>API: Route request
API->>Keys: VerifyKey(withCredits)
alt RemainingRequests NULL
Keys-->>API: Valid (no deduction)
else Limited key
Keys->>UL: Limit(KeyID, Cost)
alt Redis key exists
UL->>RC: Atomic decrement (Decrement/DecrementIfExists)
alt Sufficient credits
RC-->>UL: New remaining
UL->>UL: Buffer CreditChange (async)
UL-->>Keys: {Valid: true, Remaining}
else Insufficient
RC-->>UL: Current remaining
UL-->>Keys: {Valid: false, Remaining}
end
else Redis miss
UL->>DB: Load key credits (initializeFromDatabase)
alt Initialized in Redis
UL->>RC: SetIfNotExists(initial)
RC-->>UL: OK / race
UL->>RC: Decrement path
RC-->>UL: Decision
UL-->>Keys: Decision + Remaining
else Fallback to DB limit
UL->>DB: Direct Limit (decrement)
DB-->>UL: Decision + Remaining
UL-->>Keys: Decision + Remaining
end
end
end
API-->>LB: 200 OK (body)
LB-->>Client: 200 OK
par Async Replay
UL->>DB: Apply buffered CreditChange (replay)
DB-->>UL: Ack
end
sequenceDiagram
autonumber
participant App as API Process
participant Keys as Keys Service
participant UL as Usage Limiter
note over App: Shutdown
App->>Keys: Close()
Keys->>UL: Close()
alt Redis-backed limiter
UL-->>Keys: Drain replay buffer (timeout then flush)
else DB-only limiter
UL-->>Keys: No-op / immediate
end
Keys-->>App: Closed
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 28
🔭 Outside diff range comments (2)
go/apps/api/run.go (1)
180-190: Avoid creating duplicate Redis pools — reuse the existing counter for keys' usage limiterShort: run.go already creates a Redis-backed counter (ctr := counter.NewRedis(...)) and passes it into the ratelimit service. Passing cfg.RedisUrl into keys.Config causes keys.New -> usagelimiter.NewRedisWithCounter -> counter.NewRedis to create a second Redis client/pool. Reuse the existing counter (or usage-limiter) instead of giving keys a Redis URL.
Files to inspect/fix:
- go/apps/api/run.go
- ctr, err := counter.NewRedis(...) (creates first pool)
- keys.New(keys.Config{ ..., RedisURL: cfg.RedisUrl, }) (passes URL causing second pool)
- go/internal/services/keys/service.go
- reads config.RedisURL and calls usagelimiter.NewRedisWithCounter(...)
- go/internal/services/usagelimiter/service.go
- NewRedisWithCounter(...) calls counter.NewRedis(...) (creates the second pool)
Suggested change (minimal example):
- Replace the RedisURL field on keys.Config with a shared counter (or accept a usagelimiter.Service), and pass the already-created ctr from run.go.
Example diff sketch:
--- go/internal/services/keys/options.go
@@
- RedisURL string // Redis URL for usage limiting
- Counter counter.Counter // reuse shared Redis counter for usage limiting
--- go/apps/api/run.go
@@
- keySvc, err := keys.New(keys.Config{
- keySvc, err := keys.New(keys.Config{
Logger: logger,
DB: db,
KeyCache: caches.VerificationKeyByHash,
RateLimiter: rlSvc,
RBAC: rbac.New(),
Clickhouse: ch,
Region: cfg.Region,
RedisURL: cfg.RedisUrl,
Counter: ctr,--- go/internal/services/keys/service.go
@@
- ulSvc, err := usagelimiter.NewRedisWithCounter(usagelimiter.RedisConfig{
RedisURL: config.RedisURL,Logger: config.Logger,- })
- // Prefer reusing a provided counter to avoid creating a new Redis pool.
- if config.Counter != nil {
ulSvc, err = usagelimiter.NewWithCounter(usagelimiter.Config{Counter: config.Counter, Logger: config.Logger})- } else {
ulSvc, err = usagelimiter.NewRedisWithCounter(usagelimiter.RedisConfig{RedisURL: config.RedisURL, Logger: config.Logger})- }
If the usagelimiter package doesn't yet expose a constructor that accepts an existing counter (NewWithCounter / similar), add one so callers can reuse an existing counter instead of creating a new Redis connection.
go/internal/services/keys/service.go (1)
44-52: Consider making TTL configurableThe Redis TTL is currently hardcoded to 10 minutes. Consider making this configurable through the
Configstruct to allow different deployments to tune this value based on their usage patterns and Redis memory constraints.Add a TTL field to the Config struct:
type Config struct { Logger logging.Logger // Logger for service operations DB db.Database // Database connection RateLimiter ratelimit.Service // Rate limiting service RBAC *rbac.RBAC // Role-based access control Clickhouse clickhouse.ClickHouse // Clickhouse for telemetry Region string // Geographic region identifier RedisURL string // Redis URL for usage limiting + RedisTTL time.Duration // TTL for Redis keys (default: 10 minutes) KeyCache cache.Cache[string, db.FindKeyForVerificationRow] // Cache for key lookups }Then use it in the service initialization:
+ ttl := config.RedisTTL + if ttl == 0 { + ttl = time.Minute * 10 // default to 10 minutes + } ulSvc, err := usagelimiter.NewRedisWithCounter(usagelimiter.RedisConfig{ Logger: config.Logger, DB: config.DB, RedisURL: config.RedisURL, - TTL: time.Minute * 10, + TTL: ttl, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (20)
go/apps/api/integration/multi_node_usagelimiting/generated/usagelimit_nodes01_credits0010_cost01_load00_80_duration015/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_usagelimiting/generated/usagelimit_nodes01_credits0100_cost01_load00_90_duration030/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_usagelimiting/generated/usagelimit_nodes01_credits0100_cost01_load01_00_duration030/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_usagelimiting/generated/usagelimit_nodes01_credits0100_cost01_load02_00_duration030/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_usagelimiting/generated/usagelimit_nodes03_credits0010_cost01_load01_20_duration015/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_usagelimiting/generated/usagelimit_nodes03_credits0100_cost01_load00_90_duration030/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_usagelimiting/generated/usagelimit_nodes03_credits0100_cost01_load01_00_duration030/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_usagelimiting/generated/usagelimit_nodes03_credits0100_cost01_load02_00_duration030/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_usagelimiting/generated/usagelimit_nodes03_credits0100_cost20_load01_00_duration010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_usagelimiting/generated/usagelimit_nodes03_credits0500_cost05_load01_00_duration025/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_usagelimiting/generated/usagelimit_nodes03_credits1000_cost10_load00_90_duration020/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_usagelimiting/generated/usagelimit_nodes05_credits0010_cost01_load01_50_duration015/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_usagelimiting/generated/usagelimit_nodes05_credits0100_cost01_load00_90_duration030/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_usagelimiting/generated/usagelimit_nodes05_credits0100_cost01_load01_00_duration030/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_usagelimiting/generated/usagelimit_nodes05_credits0100_cost01_load02_00_duration030/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_usagelimiting/generated/usagelimit_nodes05_credits0100_cost20_load01_50_duration010/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_usagelimiting/generated/usagelimit_nodes05_credits0500_cost05_load02_00_duration025/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_usagelimiting/generated/usagelimit_nodes05_credits1000_cost10_load01_50_duration020/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_usagelimiting/generated/usagelimit_nodes09_credits0500_cost05_load02_00_duration015/generated_test.gois excluded by!**/generated/**go/apps/api/integration/multi_node_usagelimiting/generated/usagelimit_nodes09_credits1000_cost01_load03_00_duration015/generated_test.gois excluded by!**/generated/**
📒 Files selected for processing (19)
go/apps/api/integration/multi_node_usagelimiting/README.md(1 hunks)go/apps/api/integration/multi_node_usagelimiting/accuracy/accuracy_test.go(1 hunks)go/apps/api/integration/multi_node_usagelimiting/generate_tests/main.go(1 hunks)go/apps/api/integration/multi_node_usagelimiting/performance/performance_test.go(1 hunks)go/apps/api/integration/multi_node_usagelimiting/run.go(1 hunks)go/apps/api/routes/v2_keys_get_key/handler.go(1 hunks)go/apps/api/run.go(1 hunks)go/internal/services/keys/service.go(4 hunks)go/internal/services/keys/validation.go(1 hunks)go/internal/services/usagelimiter/interface.go(1 hunks)go/internal/services/usagelimiter/limit.go(3 hunks)go/internal/services/usagelimiter/redis.go(1 hunks)go/internal/services/usagelimiter/service.go(1 hunks)go/pkg/buffer/buffer.go(1 hunks)go/pkg/counter/interface.go(1 hunks)go/pkg/counter/redis.go(1 hunks)go/pkg/counter/redis_test.go(1 hunks)go/pkg/prometheus/metrics/usagelimiter.go(1 hunks)go/pkg/testutil/http.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-08-04T07:44:39.438Z
Learnt from: CR
PR: unkeyed/unkey#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T07:44:39.438Z
Learning: Applies to **/*_test.go : Use table-driven tests in Go
Applied to files:
go/apps/api/integration/multi_node_usagelimiting/generate_tests/main.go
📚 Learning: 2025-08-04T07:44:39.438Z
Learnt from: CR
PR: unkeyed/unkey#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T07:44:39.438Z
Learning: Applies to **/*_test.go : Organize Go integration tests with real dependencies
Applied to files:
go/apps/api/integration/multi_node_usagelimiting/generate_tests/main.go
📚 Learning: 2025-07-16T10:06:35.397Z
Learnt from: Flo4604
PR: unkeyed/unkey#3606
File: go/pkg/prometheus/metrics/database.go:29-30
Timestamp: 2025-07-16T10:06:35.397Z
Learning: In Go packages, variables defined in one file within a package (like `latencyBuckets` and `constLabels` in go/pkg/prometheus/metrics/http.go) are accessible from other files in the same package without requiring imports. This is a common pattern for sharing configuration across multiple files within a package.
Applied to files:
go/pkg/prometheus/metrics/usagelimiter.go
🧬 Code Graph Analysis (10)
go/internal/services/usagelimiter/service.go (3)
go/internal/services/keys/service.go (2)
New(43-64)Config(17-27)go/internal/services/usagelimiter/redis.go (3)
RedisConfig(25-39)NewCounter(97-135)CounterConfig(64-76)go/pkg/counter/interface.go (1)
Counter(21-126)
go/apps/api/integration/multi_node_usagelimiting/accuracy/accuracy_test.go (7)
go/pkg/testutil/flags.go (1)
SkipUnlessIntegration(35-40)go/internal/services/keys/service.go (2)
New(43-64)Config(17-27)go/internal/services/usagelimiter/service.go (2)
New(28-33)Config(20-23)go/pkg/testutil/seed/seed.go (1)
CreateApiRequest(83-91)go/pkg/ptr/pointer.go (1)
P(49-51)go/apps/api/openapi/gen.go (1)
KeysVerifyKeyCredits(235-241)go/apps/api/integration/http.go (2)
NewLoadbalancer(29-47)CallRandomNode(104-111)
go/internal/services/usagelimiter/limit.go (2)
go/pkg/prometheus/metrics/usagelimiter.go (2)
UsagelimiterDecisions(20-29)UsagelimiterCreditsProcessed(103-111)go/internal/services/usagelimiter/interface.go (1)
UsageResponse(20-23)
go/apps/api/integration/multi_node_usagelimiting/run.go (5)
go/pkg/testutil/http.go (1)
Harness(31-50)go/pkg/testutil/seed/seed.go (1)
CreateApiRequest(83-91)go/pkg/ptr/pointer.go (1)
P(49-51)go/apps/api/openapi/gen.go (1)
KeysVerifyKeyCredits(235-241)go/apps/api/integration/http.go (2)
NewLoadbalancer(29-47)CallRandomNode(104-111)
go/pkg/counter/redis_test.go (3)
go/pkg/testutil/containers/containers.go (1)
Redis(109-111)go/pkg/counter/redis.go (2)
NewRedis(46-72)RedisConfig(28-36)go/pkg/uid/uid.go (1)
TestPrefix(24-24)
go/apps/api/integration/multi_node_usagelimiting/performance/performance_test.go (6)
go/pkg/testutil/flags.go (1)
SkipUnlessIntegration(35-40)go/pkg/sim/seed.go (1)
Seed(9-9)go/pkg/testutil/seed/seed.go (1)
CreateApiRequest(83-91)go/pkg/ptr/pointer.go (1)
P(49-51)go/apps/api/openapi/gen.go (1)
KeysVerifyKeyCredits(235-241)go/apps/api/integration/http.go (2)
NewLoadbalancer(29-47)CallRandomNode(104-111)
go/pkg/counter/redis.go (1)
go/pkg/otel/tracing/trace.go (1)
Start(59-62)
go/internal/services/usagelimiter/redis.go (10)
go/pkg/db/interface.go (1)
Database(10-20)go/pkg/counter/interface.go (1)
Counter(21-126)go/internal/services/usagelimiter/interface.go (3)
Service(7-13)UsageRequest(15-18)UsageResponse(20-23)go/pkg/buffer/buffer.go (3)
Buffer(14-23)New(54-67)Config(25-29)go/internal/services/keys/service.go (2)
New(43-64)Config(17-27)go/internal/services/usagelimiter/service.go (2)
New(28-33)Config(20-23)go/pkg/otel/tracing/trace.go (1)
Start(59-62)go/pkg/prometheus/metrics/usagelimiter.go (3)
UsagelimiterFallbackOperations(71-79)UsagelimiterDecisions(20-29)UsagelimiterCreditsProcessed(103-111)go/pkg/db/key_update_credits.sql_generated.go (1)
UpdateKeyCreditsParams(25-29)go/pkg/repeat/every.go (1)
Every(11-37)
go/pkg/prometheus/metrics/usagelimiter.go (1)
go/internal/services/usagelimiter/redis.go (1)
NewCounter(97-135)
go/internal/services/keys/service.go (3)
go/internal/services/usagelimiter/service.go (1)
NewRedisWithCounter(53-69)go/internal/services/usagelimiter/redis.go (1)
RedisConfig(25-39)go/pkg/counter/redis.go (1)
RedisConfig(28-36)
🪛 golangci-lint (2.2.2)
go/internal/services/keys/validation.go
33-33: S1002: should omit comparison to bool constant, can be simplified to !k.Key.RemainingRequests.Valid
(staticcheck)
go/internal/services/usagelimiter/redis.go
[high] 185-185: G115: integer overflow conversion int64 -> int32
(gosec)
174-174: (*counterService).handleDecrementResult - result 1 (error) is always nil
(unparam)
🪛 LanguageTool
go/apps/api/integration/multi_node_usagelimiting/README.md
[grammar] ~1-~1: Use correct spacing
Context: ...ti-Node Usage Limiting Integration Tests This package contains comprehensive inte...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~3-~3: Use correct spacing
Context: ...ccuracy and performance characteristics. ## Test Structure ### Generated Tests (`ge...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~5-~5: Use correct spacing
Context: ...ance characteristics. ## Test Structure ### Generated Tests (generated/) Automati...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~7-~7: Use correct spacing
Context: ...ture ### Generated Tests (generated/) Automatically generated tests that cover...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~9-~9: There might be a mistake here.
Context: ...d tests that cover various combinations of: - Node counts: 1, 3, 5, 9 nodes - **Cred...
(QB_NEW_EN_OTHER)
[grammar] ~11-~11: There might be a mistake here.
Context: ...of: - Node counts: 1, 3, 5, 9 nodes - Credit amounts: 10, 100, 500, 1000 cre...
(QB_NEW_EN)
[grammar] ~12-~12: There might be a mistake here.
Context: ...it amounts**: 10, 100, 500, 1000 credits - Cost per request: 1, 5, 10, 20 credits...
(QB_NEW_EN)
[grammar] ~13-~13: There might be a mistake here.
Context: ...Cost per request**: 1, 5, 10, 20 credits - Load factors: 0.8x, 0.9x, 1.0x, 1.2x, ...
(QB_NEW_EN)
[grammar] ~14-~14: There might be a mistake here.
Context: ...0.8x, 0.9x, 1.0x, 1.2x, 1.5x, 2.0x, 3.0x - Test durations: 10-30 seconds ### Acc...
(QB_NEW_EN)
[grammar] ~15-~15: Use correct spacing
Context: ...3.0x - Test durations: 10-30 seconds ### Accuracy Tests (accuracy_test.go) Hig...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~17-~17: Use correct spacing
Context: ... ### Accuracy Tests (accuracy_test.go) High-concurrency tests focused on credit...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~19-~19: Use correct spacing
Context: ...sts focused on credit counting accuracy: - Race condition detection: 50-100 concu...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~21-~21: There might be a mistake here.
Context: ... detection**: 50-100 concurrent requests - Credit precision: Verify exact credit ...
(QB_NEW_EN)
[grammar] ~22-~22: There might be a mistake here.
Context: ...ision**: Verify exact credit consumption - Multi-node consistency: Ensure distrib...
(QB_NEW_EN)
[grammar] ~23-~23: There might be a mistake here.
Context: ...nsistency**: Ensure distributed accuracy - Edge cases: Single credit scenarios, h...
(QB_NEW_EN)
[grammar] ~24-~24: Use correct spacing
Context: ...e credit scenarios, high-cost operations ### Performance Tests (performance_test.go...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~26-~26: Use correct spacing
Context: ...erformance Tests (performance_test.go) Benchmarks and latency measurements: - ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~28-~28: Use correct spacing
Context: ...`) Benchmarks and latency measurements: - Throughput benchmarks: Measure maximum...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~30-~30: Use proper capitalization
Context: ...ughput benchmarks**: Measure maximum RPS - Latency analysis: P50, P95, P99 percen...
(QB_NEW_EN_OTHER_ERROR_IDS_6)
[grammar] ~31-~31: Insert the missing word
Context: ...mum RPS - Latency analysis: P50, P95, P99 percentiles - **Sustained load test...
(QB_NEW_EN_OTHER_ERROR_IDS_32)
[grammar] ~31-~31: There might be a mistake here.
Context: ...cy analysis**: P50, P95, P99 percentiles - Sustained load tests: 10-second stress...
(QB_NEW_EN_OTHER)
[grammar] ~33-~33: There might be a mistake here.
Context: ...ency scaling**: 1-100 concurrent workers ## Running Tests ### Quick Integration Tes...
(QB_NEW_EN_OTHER)
[grammar] ~35-~35: Use correct spacing
Context: ...100 concurrent workers ## Running Tests ### Quick Integration Tests (Single Node) `...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~37-~37: Use correct spacing
Context: ...## Quick Integration Tests (Single Node) bash go test -tags=integration ./apps/api/integration/multi_node_usagelimiting/generated/usagelimit_nodes01_*/ ### Full Integration Suite ```bash go test ...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~43-~43: Use correct spacing
Context: ...des01_/ ### Full Integration Suite bash go test -tags=integration_long ./apps/api/integration/multi_node_usagelimiting/generated// ### Stress Tests (9+ Nodes) bash go test...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~49-~49: Use correct spacing
Context: ...ated// ### Stress Tests (9+ Nodes) bash go test -tags=stress ./apps/api/integration/multi_node_usagelimiting/generated// ### Accuracy Tests bash go test -tags=in...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~55-~55: Use correct spacing
Context: ...ing/generated/*/ ### Accuracy Tests bash go test -tags=integration -run TestUsageLimitAccuracy ./apps/api/integration/multi_node_usagelimiting/ ### Performance Tests bash go test -tags...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~61-~61: Use correct spacing
Context: ...sagelimiting/ ### Performance Tests bash go test -tags=integration -run TestUsageLimitLatency ./apps/api/integration/multi_node_usagelimiting/ go test -tags=integration -run TestUsageLimitThroughput ./apps/api/integration/multi_node_usagelimiting/ ### Benchmarks bash go test -tags=integr...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~68-~68: Use correct spacing
Context: ..._node_usagelimiting/ ### Benchmarks bash go test -tags=integration -bench=BenchmarkUsageLimitPerformance ./apps/api/integration/multi_node_usagelimiting/ ``` ## Test Scenarios ### Accuracy Verificatio...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~74-~74: Use correct spacing
Context: ...de_usagelimiting/ ``` ## Test Scenarios ### Accuracy Verification - **Credit precis...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~76-~76: Use correct spacing
Context: ...est Scenarios ### Accuracy Verification - Credit precision: Never exceed the cre...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~78-~78: There might be a mistake here.
Context: ...ecision**: Never exceed the credit limit - Race condition handling: High concurre...
(QB_NEW_EN_OTHER)
[grammar] ~79-~79: Use hyphens correctly
Context: ...dling**: High concurrency doesn't cause over-spending - Multi-node consistency: Distributed no...
(QB_NEW_EN_OTHER_ERROR_IDS_29)
[grammar] ~82-~82: There might be a mistake here.
Context: ...cing**: Traffic distributed across nodes ### Performance Expectations - Latency:...
(QB_NEW_EN_OTHER)
[grammar] ~84-~84: Use correct spacing
Context: ...ross nodes ### Performance Expectations - Latency: P95 < 200ms, P99 < 500ms - **...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~86-~86: Ensure spelling is correct
Context: ...ance Expectations - Latency: P95 < 200ms, P99 < 500ms - Throughput: > 100 RP...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~86-~86: There might be a mistake here.
Context: ...ions - Latency: P95 < 200ms, P99 < 500ms - Throughput: > 100 RPS sustained - **Er...
(QB_NEW_EN_OTHER)
[grammar] ~87-~87: There might be a mistake here.
Context: ...ms - Throughput: > 100 RPS sustained - Error rate: < 1% under normal load ##...
(QB_NEW_EN)
[grammar] ~88-~88: Use correct spacing
Context: ...- Error rate: < 1% under normal load ### Edge Cases Tested - **Single credit exh...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~90-~90: Use correct spacing
Context: ...under normal load ### Edge Cases Tested - Single credit exhaustion: 1 credit, 10...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~92-~92: There might be a mistake here.
Context: ...ion**: 1 credit, 100 concurrent requests - High-cost operations: 20 credits per r...
(QB_NEW_EN)
[grammar] ~93-~93: There might be a mistake here.
Context: ...ost operations**: 20 credits per request - Extreme load: 3x expected request rate...
(QB_NEW_EN)
[grammar] ~94-~94: There might be a mistake here.
Context: ...Extreme load**: 3x expected request rate - Node failures: Graceful degradation (m...
(QB_NEW_EN)
[grammar] ~95-~95: Use correct spacing
Context: ...*: Graceful degradation (manual testing) ## Test Data Flow 1. Setup: Create API...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~97-~97: Use correct spacing
Context: ...tion (manual testing) ## Test Data Flow 1. Setup: Create API key with specific cr...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~99-~99: There might be a mistake here.
Context: ...reate API key with specific credit limit 2. Load generation: Concurrent requests a...
(QB_NEW_EN)
[grammar] ~100-~100: There might be a mistake here.
Context: ...oncurrent requests across multiple nodes 3. Real-time verification: Check response...
(QB_NEW_EN)
[grammar] ~101-~101: There might be a mistake here.
Context: ... verification**: Check response accuracy 4. ClickHouse validation: Verify analytic...
(QB_NEW_EN)
[grammar] ~102-~102: There might be a mistake here.
Context: ...ouse validation**: Verify analytics data 5. Database consistency: Check eventual c...
(QB_NEW_EN)
[grammar] ~103-~103: There might be a mistake here.
Context: ...onsistency**: Check eventual consistency 6. Cleanup: Automatic test resource clean...
(QB_NEW_EN)
[grammar] ~104-~104: Use correct spacing
Context: ...eanup**: Automatic test resource cleanup ## Regenerating Tests To add new test scen...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~106-~106: Use correct spacing
Context: ... resource cleanup ## Regenerating Tests To add new test scenarios: 1. Edit `gen...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~108-~108: There might be a problem here.
Context: ...rating Tests To add new test scenarios: 1. Edit generate_tests/main.go 2. Add new TestCase entries to `realistic...
(QB_NEW_EN_MERGED_MATCH)
[grammar] ~112-~112: Use correct spacing
Context: ...extremeEdgeCases 3. Run the generator: bash cd generate_tests && go generate ## Key Metrics Tracked - Success rate:...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~117-~117: Use correct spacing
Context: ... generate ``` ## Key Metrics Tracked - Success rate: Percentage of successful...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~119-~119: There might be a mistake here.
Context: ...: Percentage of successful verifications - Credit accuracy: Actual vs expected cr...
(QB_NEW_EN)
[grammar] ~120-~120: Use periods with abbreviations
Context: ...fications - Credit accuracy: Actual vs expected credit consumption - **Latency...
(QB_NEW_EN_OTHER_ERROR_IDS_34)
[grammar] ~120-~120: There might be a mistake here.
Context: ...*: Actual vs expected credit consumption - Latency distribution: Response time pe...
(QB_NEW_EN)
[grammar] ~121-~121: There might be a mistake here.
Context: ...istribution**: Response time percentiles - Node distribution: Traffic spread acro...
(QB_NEW_EN)
[grammar] ~122-~122: There might be a mistake here.
Context: ...tribution**: Traffic spread across nodes - Error patterns: Types and frequency of...
(QB_NEW_EN)
[grammar] ~123-~123: Use correct spacing
Context: ...terns**: Types and frequency of failures These tests ensure the usage limiting sy...
(QB_NEW_EN_OTHER_ERROR_IDS_5)
[grammar] ~125-~125: There might be a mistake here.
Context: ...f failures These tests ensure the usage limiting system maintains both high perf...
(QB_NEW_EN_OTHER)
[uncategorized] ~125-~125: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...he usage limiting system maintains both high performance and perfect accuracy under production-l...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[grammar] ~125-~125: There might be a mistake here.
Context: ...uction-like loads across multiple nodes.
(QB_NEW_EN)
🪛 markdownlint-cli2 (0.17.2)
go/apps/api/integration/multi_node_usagelimiting/README.md
113-113: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🔇 Additional comments (20)
go/apps/api/routes/v2_keys_get_key/handler.go (1)
117-117: No-op formatting change; LGTMThe inserted blank line does not affect control flow or error semantics.
go/pkg/counter/interface.go (1)
21-35: Verified — interface expansion accounted for; no changes requiredI scanned the repo: the Counter interface in go/pkg/counter/interface.go was expanded and the only concrete implementation found (redisCounter) implements the new methods and has a compile-time assertion.
- go/pkg/counter/interface.go — new methods present: Decrement, DecrementIfExists, IncrementIfExists, SetIfNotExists
- go/pkg/counter/redis.go
- var _ Counter = (*redisCounter)(nil) — line 25
- Increment — line 87
- Get — line 118
- Decrement — line 136
- SetIfNotExists — line 159
- DecrementIfExists — line 191
- IncrementIfExists — line 237
- Close — line 270
- MultiGet — line 283
No other receiver types with Increment were found in the repository.
go/apps/api/run.go (1)
194-195: OK: shutdown manager executes callbacks in LIFO order — no change requiredConfirmed: Shutdowns.Shutdown iterates the registered functions in reverse (for i := len(s.functions)-1; i >= 0; i--) and the tests (TestShutdownOrder, TestShutdownOrderWithMultipleRegistrations) assert the reverse/LIFO execution. Registering keySvc.Close is correct.
- go/pkg/shutdown/shutdown.go — Shutdown method (reverse iteration)
- go/pkg/shutdown/shutdown_test.go — TestShutdownOrder, TestShutdownOrderWithMultipleRegistrations
- Reference: go/apps/api/run.go — shutdowns.Register(keySvc.Close)
go/internal/services/usagelimiter/interface.go (1)
11-12: Close() implemented in both implementations — no action requiredBoth implementations implement Close():
- go/internal/services/usagelimiter/limit.go — func (s *service) Close() error { return nil } (direct DB, no-op)
- go/internal/services/usagelimiter/redis.go — func (s *counterService) Close() error { s.replayBuffer.Close(); return s.counter.Close() } (counter/Redis)
go/internal/services/keys/service.go (1)
66-69: LGTM! Proper resource cleanupGood addition of the
Close()method to ensure proper cleanup of the usage limiter resources.go/internal/services/usagelimiter/limit.go (3)
31-34: LGTM! Proper metrics instrumentation for denied requestsGood addition of metrics tracking for denied usage requests.
45-47: LGTM! Comprehensive metrics trackingExcellent addition of both decision metrics and credits processed tracking for allowed requests.
50-53: LGTM! Clean implementation of Close methodAppropriate no-op implementation for the direct DB service.
go/apps/api/integration/multi_node_usagelimiting/run.go (1)
21-29: LGTM! Well-structured test orchestration functionThe function signature is clear and provides comprehensive control over test parameters.
go/apps/api/integration/multi_node_usagelimiting/accuracy/accuracy_test.go (1)
24-75: LGTM! Comprehensive test coverageExcellent test matrix covering various scenarios from single-node to multi-node, different cost levels, and extreme load conditions.
go/pkg/counter/redis.go (3)
135-156: LGTM! Consistent implementation with IncrementThe
Decrementmethod follows the same pattern asIncrement, properly handling TTL for new keys.
174-218: LGTM! Robust atomic operation with proper error handlingExcellent implementation using Lua script for atomicity. The result parsing is thorough with proper type checking.
220-264: LGTM! Consistent implementation with DecrementIfExistsThe
IncrementIfExistsimplementation follows the same robust pattern asDecrementIfExists.go/pkg/counter/redis_test.go (5)
447-503: LGTM! Well-structured test for the new Decrement functionality.The test is well-designed with good coverage of basic decrement operations, TTL behavior, and concurrent access. The test cases properly verify both positive and negative counter values.
505-537: LGTM! Excellent concurrent testing with proper synchronization.The concurrent test is well-structured with proper worker synchronization and accurate expected value calculation. Good use of deterministic initial values to verify correctness.
539-638: LGTM! Comprehensive test coverage for DecrementIfExists.Excellent test coverage including non-existent keys, existing keys, negative values, and concurrent operations with proper success tracking. The synchronization and validation logic is correct.
640-734: LGTM! Excellent usage limiter scenario testing.The
UsageLimiterScenariotest case (lines 700-734) perfectly simulates the real-world usage pattern of the usage limiter, including the transition from non-existent to existing keys and proper handling of insufficient credits. This validates the critical path for the Redis-backed usage limiter implementation.
736-778: LGTM! Good mixed operations test.The test properly validates state transitions across different operation types and ensures correct final values through the sequence of operations.
go/apps/api/integration/multi_node_usagelimiting/performance/performance_test.go (1)
334-345: Potential data race when accessing shared variables.The concurrent access to
requestCountanderrorCountinside the goroutine could lead to incorrect counts. The mutex lock should encompass the entire if-else block.for time.Since(start) < testDuration { res, callErr := integration.CallRandomNode[handler.Request, handler.Response]( lb, "POST", "/v2/keys.verifyKey", headers, req) mu.Lock() if callErr != nil || res.Status != 200 { errorCount++ } else { requestCount++ } mu.Unlock() }Likely an incorrect or invalid review comment.
go/pkg/prometheus/metrics/usagelimiter.go (1)
1-113: LGTM! Well-structured Prometheus metrics for usage limiter monitoring.The metrics are well-designed with appropriate labels, clear documentation, and helpful usage examples. The metrics cover all critical aspects of the usage limiter: decisions, replays, fallbacks, Redis operations, and credits processing.
go/apps/api/integration/multi_node_usagelimiting/accuracy/accuracy_test.go
Outdated
Show resolved
Hide resolved
go/apps/api/integration/multi_node_usagelimiting/accuracy/accuracy_test.go
Outdated
Show resolved
Hide resolved
go/apps/api/integration/multi_node_usagelimiting/generate_tests/main.go
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
go/apps/api/integration/multi_node_usagelimiting/accuracy/accuracy_test.go
Outdated
Show resolved
Hide resolved
go/apps/api/integration/multi_node_usagelimiting/accuracy/accuracy_test.go
Outdated
Show resolved
Hide resolved
go/apps/api/integration/multi_node_usagelimiting/performance/performance_test.go
Outdated
Show resolved
Hide resolved
go/apps/api/integration/multi_node_usagelimiting/performance/performance_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
go/pkg/counter/redis.go (2)
98-111: Potential nil-pointer panic when Logger is omitted; default the logger.Config advertises Logger as optional but methods call Debug/Error/Warn unconditionally. Default to a no-op or standard logger to avoid panics.
Apply:
- rdb := redis.NewClient(opts) - config.Logger.Debug("pinging redis") + // Default logger if none provided + logger := config.Logger + if logger == nil { + logger = logging.New() + } + + rdb := redis.NewClient(opts) + logger.Debug("pinging redis") @@ - return &redisCounter{ - redis: rdb, - logger: config.Logger, - }, nil + return &redisCounter{ + redis: rdb, + logger: logger, + }, nil
136-144: Fix TTL race/heuristic in Increment; set TTL atomically only when key is new.The “newValue == value” heuristic is unreliable (e.g., pre-existing key at 0). Use a Lua script to INCRBY and set TTL only on first creation.
Apply:
- // Increment the counter - newValue, err := r.redis.IncrBy(ctx, key, value).Result() - if err != nil { - return 0, err - } - - // If TTL is provided and this is a new key (value == increment amount), - // set the expiration time - if len(ttl) > 0 && newValue == value { - if err := r.redis.Expire(ctx, key, ttl[0]).Err(); err != nil { - r.logger.Error("failed to set TTL on counter", "key", key, "error", err.Error()) - // We don't return the error since the increment operation was successful - } - } - - return newValue, nil + // Increment; if TTL provided, set it atomically only on first creation via Lua + var newValue int64 + if len(ttl) > 0 { + ttlMs := ttl[0].Milliseconds() + res, err := incrementIfNewScriptCached.Run(ctx, r.redis, []string{key}, value, ttlMs).Result() + if err != nil { + return 0, err + } + parsed, err := parseNumericValue(res) + if err != nil { + return 0, fmt.Errorf("increment: unexpected script result: %w", err) + } + newValue = parsed + } else { + var err error + newValue, err = r.redis.IncrBy(ctx, key, value).Result() + if err != nil { + return 0, err + } + } + return newValue, nilAdd these at package scope (outside the diffed range):
// Place near other script constants const incrementIfNewScript = ` local key = KEYS[1] local increment = tonumber(ARGV[1]) local ttl_ms = tonumber(ARGV[2]) if redis.call('EXISTS', key) == 0 then local newValue = redis.call('INCRBY', key, increment) if ttl_ms and ttl_ms > 0 then redis.call('PEXPIRE', key, ttl_ms) end return newValue else return redis.call('INCRBY', key, increment) end ` var incrementIfNewScriptCached = redis.NewScript(incrementIfNewScript)
♻️ Duplicate comments (11)
go/internal/services/usagelimiter/limit.go (1)
31-33: Insufficient-credits guard reads well (matches earlier guidance).The simplified check
if req.Cost > 0 && remaining < req.Costis correct and removes duplicated logic from the prior version.go/pkg/counter/redis.go (1)
223-231: Validate decrement amount > 0 before invoking the Lua script.Negative/zero values invert semantics and make script behavior ambiguous.
Apply:
func (r *redisCounter) DecrementIfExists(ctx context.Context, key string, value int64) (int64, bool, bool, error) { ctx, span := tracing.Start(ctx, "RedisCounter.DecrementIfExists") defer span.End() + if value <= 0 { + return 0, false, false, fmt.Errorf("counter: decrement value must be positive") + }go/internal/services/keys/validation.go (1)
32-35: Early exit for unlimited usage (NULL remaining) is correct and idiomatic.Skipping the limiter when RemainingRequests.Valid is false matches the service contract.
go/pkg/counter/interface.go (1)
116-129: SetIfNotExists: clarify TTL precedence with variadic argsMirror Increment's TTL semantics in the comment by stating that only the first TTL (if provided) is used.
Apply this diff:
-// - ttl: Optional time-to-live duration for the counter +// - ttl: Optional time-to-live duration for the counter. If multiple are provided, only the first is used.go/apps/api/integration/multi_node_usagelimiting/performance/performance_test.go (1)
83-107: Consider adding error aggregation for better diagnostics.While the test correctly validates each request, consider aggregating error types to provide better diagnostics when failures occur. This would help identify patterns in failures during performance testing.
// Run simple performance test with fixed number of requests numRequests := 100 successCount := 0 + var errorTypes = make(map[string]int) start := time.Now() for i := 0; i < numRequests; i++ { res, callErr := integration.CallRandomNode[handler.Request, handler.Response]( lb, "POST", "/v2/keys.verifyKey", headers, req) - require.NoError(t, callErr, "Request %d failed", i) - require.Equal(t, 200, res.Status, "Request %d got wrong status", i) + if callErr != nil { + errorTypes[callErr.Error()]++ + t.Errorf("Request %d failed: %v", i, callErr) + continue + } + if res.Status != 200 { + errorTypes[fmt.Sprintf("HTTP %d", res.Status)]++ + t.Errorf("Request %d got wrong status: %d", i, res.Status) + continue + } if res.Body.Data.Valid { successCount++ } } duration := time.Since(start) rps := float64(numRequests) / duration.Seconds() t.Logf("Performance test for %d nodes: %d requests in %v (%.2f RPS), %d successful", nodeCount, numRequests, duration, rps, successCount) + if len(errorTypes) > 0 { + t.Logf("Error breakdown: %v", errorTypes) + }go/apps/api/integration/multi_node_usagelimiting/decrement/decrement_test.go (1)
175-175: Consider using require.Len for metrics validationFor consistency with testify conventions.
- require.Equal(t, tc.nodeCount, len(lbMetrics), "All nodes should have received traffic") + require.Len(t, lbMetrics, tc.nodeCount, "All nodes should have received traffic")go/internal/services/usagelimiter/redis.go (5)
21-22: Fix typo in constant nameThe constant name has a typo that should be corrected.
const ( defaultTTL = 10 * time.Minute - defaultReplayWorkers = 8 + defaultReplayWorkers = 8 )
92-111: NewCounter documentation is comprehensiveThe function documentation clearly describes the counter-based implementation characteristics, benefits, and parameters. Consider documenting the default ReplayWorkers value of 8 in the doc comment for better discoverability.
Add to the function documentation:
// Returns: // - Service: Counter-based implementation of the Service interface // - error: Any initialization errors +// +// Note: ReplayWorkers defaults to 8 if not specified in the config.
112-117: Consider using the assert package more idiomaticallyThe validation approach could be more idiomatic.
if err := assert.All( - assert.NotNil(config.Counter), - assert.NotNil(config.DB), + assert.NotNil(config.Counter, "Counter is required"), + assert.NotNil(config.DB, "DB is required"), ); err != nil { return nil, err }
293-302: Consider adding metrics for replay errorsThe error is logged but not tracked in metrics for observability.
func (s *counterService) replayRequests() { for change := range s.replayBuffer.Consume() { err := s.syncWithDB(context.Background(), change) if err != nil { + metrics.UsagelimiterReplayOperations.WithLabelValues("replay_error").Inc() s.logger.Error("failed to replay credit change", "error", err) } } }
329-389: Consider simplifying the Close methodThe graceful shutdown logic could potentially be simplified by moving it entirely into Close() instead of having a separate GracefulShutdown concept.
The current implementation handles shutdown comprehensively with timeout, buffer draining, and active processing of remaining items. This is good for data integrity. Consider if the complexity is warranted for your use case, or if a simpler approach would suffice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
go/apps/api/integration/multi_node_usagelimiting/decrement/decrement_test.go(1 hunks)go/apps/api/integration/multi_node_usagelimiting/performance/performance_test.go(1 hunks)go/apps/api/routes/v2_keys_verify_key/200_test.go(1 hunks)go/internal/services/keys/validation.go(2 hunks)go/internal/services/usagelimiter/limit.go(3 hunks)go/internal/services/usagelimiter/redis.go(1 hunks)go/pkg/counter/interface.go(1 hunks)go/pkg/counter/redis.go(2 hunks)go/pkg/counter/redis_test.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.go: Follow comprehensive documentation guidelines for Go code as described in go/GO_DOCUMENTATION_GUIDELINES.md
Every public function/type in Go code must be documented
Prefer interfaces for testability in Go code
Use AIDEV-* comments for complex/important code in Go services
Files:
go/apps/api/routes/v2_keys_verify_key/200_test.gogo/apps/api/integration/multi_node_usagelimiting/decrement/decrement_test.gogo/internal/services/keys/validation.gogo/apps/api/integration/multi_node_usagelimiting/performance/performance_test.gogo/pkg/counter/redis_test.gogo/internal/services/usagelimiter/limit.gogo/internal/services/usagelimiter/redis.gogo/pkg/counter/redis.gogo/pkg/counter/interface.go
**/*_test.go
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*_test.go: Use table-driven tests in Go
Organize Go integration tests with real dependencies
Organize Go tests by HTTP status codes
Files:
go/apps/api/routes/v2_keys_verify_key/200_test.gogo/apps/api/integration/multi_node_usagelimiting/decrement/decrement_test.gogo/apps/api/integration/multi_node_usagelimiting/performance/performance_test.gogo/pkg/counter/redis_test.go
**/*.{env,js,ts,go}
📄 CodeRabbit Inference Engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
go/apps/api/routes/v2_keys_verify_key/200_test.gogo/apps/api/integration/multi_node_usagelimiting/decrement/decrement_test.gogo/internal/services/keys/validation.gogo/apps/api/integration/multi_node_usagelimiting/performance/performance_test.gogo/pkg/counter/redis_test.gogo/internal/services/usagelimiter/limit.gogo/internal/services/usagelimiter/redis.gogo/pkg/counter/redis.gogo/pkg/counter/interface.go
🧠 Learnings (5)
📚 Learning: 2025-08-19T09:46:03.679Z
Learnt from: Flo4604
PR: unkeyed/unkey#3800
File: go/internal/services/usagelimiter/redis.go:176-186
Timestamp: 2025-08-19T09:46:03.679Z
Learning: Zero-cost requests in the usage limiter should not change the remaining credits but should still return the actual remaining credits for that key, not treat the key as unlimited. A key with 100 credits remaining should still report 100 credits remaining after a zero-cost request.
Applied to files:
go/apps/api/routes/v2_keys_verify_key/200_test.gogo/internal/services/keys/validation.gogo/internal/services/usagelimiter/limit.gogo/internal/services/usagelimiter/redis.go
📚 Learning: 2025-08-19T09:42:40.897Z
Learnt from: Flo4604
PR: unkeyed/unkey#3800
File: go/internal/services/keys/validation.go:45-52
Timestamp: 2025-08-19T09:42:40.897Z
Learning: In go/internal/services/keys/validation.go, keys with unlimited usage (RemainingRequests.Valid = false) have an early return that bypasses the usage limiter entirely. The usage limiter is only called for keys with finite remaining request counts.
Applied to files:
go/internal/services/keys/validation.gogo/apps/api/integration/multi_node_usagelimiting/performance/performance_test.gogo/internal/services/usagelimiter/limit.gogo/internal/services/usagelimiter/redis.go
📚 Learning: 2025-08-04T07:44:39.438Z
Learnt from: CR
PR: unkeyed/unkey#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-04T07:44:39.438Z
Learning: Applies to **/*_test.go : Organize Go tests by HTTP status codes
Applied to files:
go/apps/api/integration/multi_node_usagelimiting/performance/performance_test.go
📚 Learning: 2025-08-19T09:36:19.442Z
Learnt from: Flo4604
PR: unkeyed/unkey#3800
File: go/pkg/counter/redis.go:241-265
Timestamp: 2025-08-19T09:36:19.442Z
Learning: In Redis counter implementations using go-redis client, numeric values should not be returned as []byte. The go-redis client handles type conversion automatically, and if []byte appears for numeric data, it indicates a configuration problem rather than expected behavior that should be handled.
Applied to files:
go/internal/services/usagelimiter/redis.gogo/pkg/counter/redis.go
📚 Learning: 2025-08-19T08:57:31.750Z
Learnt from: Flo4604
PR: unkeyed/unkey#3800
File: go/apps/api/integration/multi_node_usagelimiting/run.go:107-126
Timestamp: 2025-08-19T08:57:31.750Z
Learning: Go 1.22+ supports "range over integers" syntax: `for range N` iterates N times, and `for i := range N` iterates with i from 0 to N-1. This is valid Go syntax and should not be flagged as a compilation error.
Applied to files:
go/internal/services/usagelimiter/redis.go
🧬 Code Graph Analysis (7)
go/apps/api/integration/multi_node_usagelimiting/decrement/decrement_test.go (5)
go/pkg/testutil/flags.go (1)
SkipUnlessIntegration(35-40)go/pkg/testutil/seed/seed.go (1)
CreateApiRequest(83-91)go/pkg/ptr/pointer.go (1)
P(49-51)go/apps/api/openapi/gen.go (1)
KeysVerifyKeyCredits(235-241)go/apps/api/integration/http.go (2)
NewLoadbalancer(29-47)CallRandomNode(104-111)
go/apps/api/integration/multi_node_usagelimiting/performance/performance_test.go (5)
go/pkg/testutil/flags.go (1)
SkipUnlessIntegration(35-40)go/pkg/testutil/seed/seed.go (1)
CreateApiRequest(83-91)go/pkg/ptr/pointer.go (1)
P(49-51)go/apps/api/openapi/gen.go (1)
KeysVerifyKeyCredits(235-241)go/apps/api/integration/http.go (2)
NewLoadbalancer(29-47)CallRandomNode(104-111)
go/pkg/counter/redis_test.go (3)
go/pkg/testutil/containers/containers.go (1)
Redis(109-111)go/pkg/counter/redis.go (2)
NewRedis(85-111)RedisConfig(67-75)go/pkg/uid/uid.go (1)
TestPrefix(24-24)
go/internal/services/usagelimiter/limit.go (2)
go/pkg/prometheus/metrics/usagelimiter.go (2)
UsagelimiterDecisions(20-29)UsagelimiterCreditsProcessed(86-94)go/internal/services/usagelimiter/interface.go (1)
UsageResponse(20-23)
go/internal/services/usagelimiter/redis.go (9)
go/pkg/db/interface.go (1)
Database(10-20)go/pkg/counter/interface.go (1)
Counter(21-147)go/internal/services/usagelimiter/interface.go (3)
Service(7-13)UsageRequest(15-18)UsageResponse(20-23)go/pkg/buffer/buffer.go (3)
Buffer(14-23)New(54-67)Config(25-29)go/internal/services/usagelimiter/service.go (2)
New(25-30)Config(17-20)go/pkg/circuitbreaker/lib.go (1)
WithLogger(103-107)go/pkg/db/handle_err_no_rows.go (1)
IsNotFound(8-10)go/pkg/db/key_update_credits_decrement.sql_generated.go (1)
UpdateKeyCreditsDecrementParams(22-25)go/pkg/repeat/every.go (1)
Every(11-37)
go/pkg/counter/redis.go (1)
go/pkg/otel/tracing/trace.go (1)
Start(59-62)
go/pkg/counter/interface.go (1)
go/apps/api/openapi/gen.go (1)
Decrement(22-22)
🪛 golangci-lint (2.2.2)
go/pkg/counter/redis_test.go
808-808: TestRedisCounterDecrementLogic$2$1 - id is unused
(unparam)
go/internal/services/usagelimiter/redis.go
[high] 220-220: G115: integer overflow conversion int64 -> int32
(gosec)
[high] 225-225: G115: integer overflow conversion int64 -> int32
(gosec)
209-209: (*counterService).handleResult - result 1 (error) is always nil
(unparam)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test API / API Test Local
- GitHub Check: Test Go API Local / Test
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (19)
go/internal/services/usagelimiter/limit.go (3)
44-46: Metrics instrumentation on approval path looks correct.
- Decisions counter labeled as ("db", "allowed") is appropriate.
- Credits processed uses the request cost (float64 cast), as expected.
49-51: Close() method is appropriate for parity with the new interface.Direct DB implementation has nothing to release; returning nil is correct.
46-46: Compilation issue: undefined identifier ‘max’.Go doesn’t have a built-in max for integers, and there’s no local helper in this file. This will not compile.
Apply:
- metrics.UsagelimiterCreditsProcessed.Add(float64(req.Cost)) - return UsageResponse{Valid: true, Remaining: max(0, remaining-req.Cost)}, nil + metrics.UsagelimiterCreditsProcessed.Add(float64(req.Cost)) + remainingAfter := remaining - req.Cost + if remainingAfter < 0 { + remainingAfter = 0 + } + return UsageResponse{Valid: true, Remaining: remainingAfter}, nil⛔ Skipped due to learnings
Learnt from: Flo4604 PR: unkeyed/unkey#3800 File: go/apps/api/integration/multi_node_usagelimiting/accuracy/accuracy_test.go:231-240 Timestamp: 2025-08-19T08:53:43.927Z Learning: Go 1.21+ includes builtin min() and max() functions for ordered types including integers, so min/max calls in Go code should not be flagged as undefined when the project uses Go 1.21 or later.go/pkg/counter/redis.go (3)
15-47: Lua decrement script is solid: atomic, preserves TTL, and explicit success flag.
- Uses DECRBY to preserve TTL (no SET TTL wipe).
- Clear tri-state return: {value, existed, success} avoids sentinel ambiguity.
49-52: Good: cached script object name clarifies intent and uses EVALSHA under the hood.This addresses prior nit about misleading “SHA” naming.
258-282: parseNumericValue coverage and messages are good; no need to add []byte handling.Matches expected go-redis return types; aligns with prior clarification that numeric values won’t be []byte.
go/apps/api/routes/v2_keys_verify_key/200_test.go (1)
184-184: Expectation update to preserve actual remaining credits is correct.When cost exceeds available credits, the response should reflect the actual remaining (5), not force it to 0. This aligns with limiter semantics and earlier product guidance.
go/internal/services/keys/validation.go (1)
45-47: Always populating remaining with the limiter’s value fixes the “exceeded but keep current” case.This ensures scenarios like cost=10 with 5 credits report 5 remaining while marking the key invalid.
go/pkg/counter/redis_test.go (1)
609-646: Concurrency decrement test counts actual successes; assertion is correct.Counting only existed && success avoids overcounting when decrements are refused.
go/pkg/counter/interface.go (3)
62-76: Documentation is accurate and comprehensiveThe Decrement method documentation correctly states that it mirrors Increment behavior, creates the counter if it doesn't exist, and honors only the first TTL argument. This is well documented.
78-114: DecrementIfExists documentation is comprehensive and clearThe method documentation thoroughly covers all return scenarios, behavior guarantees, and provides clear usage patterns. The explicit success flag eliminates ambiguity about operation results.
130-140: Delete method is correctly documentedThe Delete method has clear documentation about its purpose and parameters.
go/apps/api/integration/multi_node_usagelimiting/performance/performance_test.go (2)
23-44: Test structure looks goodThe test is well-organized with clear test cases covering different node counts and cost configurations. The parameterized approach provides good coverage across various scenarios.
109-197: Comprehensive throughput test with appropriate metricsThe test properly measures sustained load with concurrent workers, tracks errors and throughput, and includes reasonable performance thresholds. The test structure and metrics collection are well-implemented.
go/apps/api/integration/multi_node_usagelimiting/decrement/decrement_test.go (2)
23-209: TestDecrementAccuracy is comprehensive and well-structuredThe test thoroughly validates decrement accuracy across different scenarios with proper concurrency handling, result validation, and database consistency checks. The test includes:
- Parameterized test cases covering various node counts, credits, and costs
- Concurrent request execution with realistic jitter
- Exact accuracy validation for successes/failures
- Multi-node traffic distribution verification
- Database consistency verification with polling
211-368: Edge cases are well coveredThe test suite covers important edge cases:
- Zero credit handling
- Exact credit depletion
- High-cost concurrency
All test scenarios are properly implemented with appropriate assertions.
go/internal/services/usagelimiter/redis.go (3)
25-33: CreditChange documentation is clear and accurateThe exported type has proper documentation that clearly describes its purpose and fields.
251-280: Excellent handling of Redis initialization to avoid negative valuesThe code correctly prevents negative values from being written to Redis during initialization. The logic for determining the appropriate initialization value based on credit sufficiency is well thought out and maintains data integrity.
304-327: syncWithDB implementation looks good with proper metricsThe function correctly tracks latency and success/error metrics for replay operations. The circuit breaker protection and timeout handling are appropriate.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
go/pkg/db/database.go (1)
133-139: Fix double-close when read replica shares the same DB handle as writeWhen ReadOnlyDSN is not provided, readReplica.db and writeReplica.db point to the same *sql.DB. Close() currently closes both, causing a second close on the same handle. Only close the read replica if it’s a distinct connection.
Apply this diff:
// Only close the read replica if it's a separate connection -if d.readReplica != nil { - readCloseErr := d.readReplica.db.Close() - if readCloseErr != nil { - return fault.Wrap(readCloseErr) - } -} +if d.readReplica != nil && d.readReplica.db != d.writeReplica.db { + if readCloseErr := d.readReplica.db.Close(); readCloseErr != nil { + return fault.Wrap(readCloseErr) + } +}go/pkg/counter/redis.go (2)
86-111: Add timeout to Redis ping; optionally expose pool/timeouts in configAvoid indefinite hangs on startup and consider tuning for high-throughput usage limiting.
Apply:
- // Test connection - _, err = rdb.Ping(context.Background()).Result() + // Test connection with timeout + pingCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + _, err = rdb.Ping(pingCtx).Result() if err != nil { return nil, fmt.Errorf("failed to ping redis: %w", err) }Optionally, extend RedisConfig with optional PoolSize/MinIdleConns/Dial/Read/Write timeouts and propagate them into opts to better handle load spikes. I can draft that if you want.
321-360: Reuse parseNumericValue in MultiGet and improve diagnosticsAvoid special-casing strings; use the existing converter for consistency and clearer warnings.
Apply:
- s, ok := values[i].(string) - if !ok { - r.logger.Warn("unexpected type for counter value", - "key", key, - "type", fmt.Sprintf("%T", values[i]), - ) - continue - } - v, err := strconv.ParseInt(s, 10, 64) + v, err := parseNumericValue(values[i]) if err != nil { r.logger.Warn("failed to parse counter value", "key", key, - "value", s, - "error", err, + "type", fmt.Sprintf("%T", values[i]), + "error", err.Error(), ) continue } result[key] = vAlso applies to: 338-356
go/pkg/counter/redis_test.go (2)
227-248: Fix invalid range-over-int loops in InterleavedOperationsGo cannot range over integers. Use classic for loops for workers and operations.
Apply:
- for i := range numWorkers { + for i := 0; i < numWorkers; i++ { go func(id int) { defer wg.Done() - for j := range operationsPerWorker { + for j := 0; j < operationsPerWorker; j++ {Also applies to: 231-247
262-294: TTL tests may be flaky with heuristic-based TTL settingGiven Increment/Decrement currently set TTL post-hoc via heuristic, concurrent cases could miss TTL. Once the atomic TTL scripts are applied in redis.go, these tests become reliable. For now, consider small Eventually/Retry or increased TTL buffer to reduce flakiness.
Also applies to: 487-505
♻️ Duplicate comments (4)
go/pkg/counter/redis.go (4)
137-144: Fix TTL race/heuristic on Increment; make TTL setting atomic when key is createdThe heuristic newValue == value is unsafe (existing key at 0, races). Set TTL atomically only on initial creation.
Apply:
- // If TTL is provided and this is a new key (value == increment amount), - // set the expiration time - if len(ttl) > 0 && newValue == value { - if err := r.redis.Expire(ctx, key, ttl[0]).Err(); err != nil { - r.logger.Error("failed to set TTL on counter", "key", key, "error", err.Error()) - // We don't return the error since the increment operation was successful - } - } + // If TTL is provided, handle atomically: set TTL only if the key didn't exist + if len(ttl) > 0 { + ttlMs := ttl[0].Milliseconds() + res, err := incrementIfNewScriptCached.Run(ctx, r.redis, []string{key}, value, ttlMs).Result() + if err != nil { + return 0, err + } + parsed, err := parseNumericValue(res) + if err != nil { + return 0, fmt.Errorf("increment: unexpected script result: %w", err) + } + return parsed, nil + }Add at package scope:
const incrementIfNewScript = ` local key = KEYS[1] local incr = tonumber(ARGV[1]) local ttl_ms = tonumber(ARGV[2]) if redis.call('EXISTS', key) == 0 then local newValue = incr redis.call('SET', key, newValue, 'PX', ttl_ms, 'NX') return newValue else return redis.call('INCRBY', key, incr) end ` var incrementIfNewScriptCached = redis.NewScript(incrementIfNewScript)
175-196: Fix TTL race/heuristic on Decrement; make TTL setting atomic when key is createdThe heuristic newValue == -value misclassifies existing-at-zero and races; use a script to set TTL only on creation.
Apply:
- // Decrement the counter - newValue, err := r.redis.DecrBy(ctx, key, value).Result() - if err != nil { - return 0, err - } - - // If TTL is provided and this appears to be a new key (negative value after decrement), - // set the expiration time - if len(ttl) > 0 && newValue == -value { - if err := r.redis.Expire(ctx, key, ttl[0]).Err(); err != nil { - r.logger.Error("failed to set TTL on counter", "key", key, "error", err.Error()) - // We don't return the error since the decrement operation was successful - } - } - - return newValue, nil + // Decrement; if TTL provided, set it only when key is created (atomically via Lua) + if len(ttl) > 0 { + ttlMs := ttl[0].Milliseconds() + res, err := decrementIfNewScriptCached.Run(ctx, r.redis, []string{key}, value, ttlMs).Result() + if err != nil { + return 0, err + } + parsed, err := parseNumericValue(res) + if err != nil { + return 0, fmt.Errorf("decrement: unexpected script result: %w", err) + } + return parsed, nil + } + + return r.redis.DecrBy(ctx, key, value).Result()Add at package scope:
const decrementIfNewScript = ` local key = KEYS[1] local decr = tonumber(ARGV[1]) local ttl_ms = tonumber(ARGV[2]) if redis.call('EXISTS', key) == 0 then local newValue = 0 - decr redis.call('SET', key, newValue, 'PX', ttl_ms, 'NX') return newValue else return redis.call('DECRBY', key, decr) end ` var decrementIfNewScriptCached = redis.NewScript(decrementIfNewScript)
198-212: Optional: validate initial value for SetIfNotExists (domain-specific)If your counters must be non-negative on init, reject negative values early.
Apply:
func (r *redisCounter) SetIfNotExists(ctx context.Context, key string, value int64, ttl ...time.Duration) (bool, error) { ctx, span := tracing.Start(ctx, "RedisCounter.SetIfNotExists") defer span.End() + if value < 0 { + return false, fmt.Errorf("counter: initial value must be non-negative") + }
223-257: Validate input: require positive decrement amountAvoid calling the script with invalid values; enforce value > 0.
Apply:
func (r *redisCounter) DecrementIfExists(ctx context.Context, key string, value int64) (int64, bool, bool, error) { ctx, span := tracing.Start(ctx, "RedisCounter.DecrementIfExists") defer span.End() + if value <= 0 { + return 0, false, false, fmt.Errorf("counter: decrement value must be positive") + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
go/pkg/counter/redis.go(3 hunks)go/pkg/counter/redis_test.go(3 hunks)go/pkg/db/database.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*.go: Follow comprehensive documentation guidelines for Go code as described in go/GO_DOCUMENTATION_GUIDELINES.md
Every public function/type in Go code must be documented
Prefer interfaces for testability in Go code
Use AIDEV-* comments for complex/important code in Go services
Files:
go/pkg/db/database.gogo/pkg/counter/redis.gogo/pkg/counter/redis_test.go
**/*.{env,js,ts,go}
📄 CodeRabbit Inference Engine (CLAUDE.md)
All environment variables must follow the format: UNKEY_<SERVICE_NAME>_VARNAME
Files:
go/pkg/db/database.gogo/pkg/counter/redis.gogo/pkg/counter/redis_test.go
**/*_test.go
📄 CodeRabbit Inference Engine (CLAUDE.md)
**/*_test.go: Use table-driven tests in Go
Organize Go integration tests with real dependencies
Organize Go tests by HTTP status codes
Files:
go/pkg/counter/redis_test.go
🧠 Learnings (1)
📚 Learning: 2025-08-19T09:36:19.442Z
Learnt from: Flo4604
PR: unkeyed/unkey#3800
File: go/pkg/counter/redis.go:241-265
Timestamp: 2025-08-19T09:36:19.442Z
Learning: In Redis counter implementations using go-redis client, numeric values should not be returned as []byte. The go-redis client handles type conversion automatically, and if []byte appears for numeric data, it indicates a configuration problem rather than expected behavior that should be handled.
Applied to files:
go/pkg/counter/redis.go
🧬 Code Graph Analysis (3)
go/pkg/db/database.go (2)
go/pkg/assert/all.go (1)
All(19-26)go/pkg/fault/wrap.go (2)
Wrap(25-67)Internal(75-89)
go/pkg/counter/redis.go (2)
go/pkg/otel/tracing/trace.go (1)
Start(59-62)go/apps/api/run.go (1)
Run(33-262)
go/pkg/counter/redis_test.go (3)
go/pkg/testutil/containers/containers.go (1)
Redis(109-111)go/pkg/counter/redis.go (2)
NewRedis(85-112)RedisConfig(67-75)go/pkg/uid/uid.go (1)
TestPrefix(24-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Test Go API Local / Test
- GitHub Check: Test API / API Test Local
- GitHub Check: Build / Build
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
go/pkg/db/database.go (1)
10-10: LGTM: importing assert for fail-fast config validationThe addition cleanly enables precondition checks in New(). No concerns.
go/pkg/counter/redis.go (2)
49-52: Nice: use redis.NewScript cached object and clear namingRenaming to decrementIfExistsScriptCached clarifies it’s a script object, not a SHA. Good organization.
259-283: parseNumericValue: solid coverage without []byte (consistent with go-redis)Covers int64/float64/string/int/int32 and does overflow checks. Excluding []byte aligns with expected go-redis return types (per prior learnings from this PR).
go/pkg/counter/redis_test.go (4)
297-324: Connection tests rely on local Redis; ensure CI env provides itcontainers.Redis(t) returns redis://localhost:6379. Confirm CI has Redis listening there or switch to testcontainers for isolation.
Would you like a testcontainers-based helper to spin up Redis for integration tests?
649-727: Nice coverage for Delete scenariosDeleteExistingKey/NonExistent/Reinitialize cover common flows and interaction with DecrementIfExists.
731-917: Decrement semantics are well tested (sufficient/not sufficient/concurrency)The suite validates non-negativity invariant from the Lua path and concurrent correctness. Once loop fixes are applied, these give strong confidence.
806-824: Fix invalid range-over-int in ConcurrentDecrement (decrement logic tests)Same issue: cannot range over an int value.
Apply:
- for range numGoroutines { + for i := 0; i < numGoroutines; i++ { wg.Add(1) go func() { defer wg.Done()⛔ Skipped due to learnings
Learnt from: Flo4604 PR: unkeyed/unkey#3800 File: go/apps/api/integration/multi_node_usagelimiting/run.go:107-126 Timestamp: 2025-08-19T08:57:31.750Z Learning: Go 1.22+ supports "range over integers" syntax: `for range N` iterates N times, and `for i := range N` iterates with i from 0 to N-1. This is valid Go syntax and should not be flagged as a compilation error.
|
|
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
it wasn't broken but OK 🙂 |
|
Nevermind it was broken... butttt I think that's my error because I didn't want 5's included? |
|
In short, the comment is correct.. the code is not. 😬 My mistake! It's not a huge deal, so i'll fix it another time. |
What does this PR do?
Use Redis as usagelimiter instead of calling the database each time.
We are now using a usagelimiter that relies on redis to track the credits remaining, meaning we are accurate per region and there could be drift between regions but its better for latency than calling the db each time.
The flow looks like this now:
graph TD %% Old System (Database-based) subgraph OLD["Old System: Database-based Usage Limiting"] A1[API Request] --> B1{Key Validation} B1 --> C1[Database Query] C1 --> D1[Read Current Credits] D1 --> E1{Sufficient Credits?} E1 -->|Yes| F1[Database UPDATE<br/>Credits = Credits - Cost] E1 -->|No| G1[Deny Request] F1 --> H1[Return Success] %% Problems with old system I1[Issues:<br/>Database bottleneck<br/>Race conditions<br/>High latency<br/>Scaling issues] end %% New System (Redis-based) subgraph NEW["New System: Redis-based Atomic Counters"] A2[API Request] --> B2{Key in Redis?} B2 -->|Yes| C2[Lua Script:<br/>DecrementIfExists] C2 --> D2{Script Result} D2 -->|SUCCESS| E2[Buffer Credit Change<br/>for Async DB Update] D2 -->|INSUFFICIENT| F2[Deny Request] E2 --> G2[Return Success] B2 -->|No| H2[Load from Database] H2 --> I2[SetIfNotExists<br/>Credits minus Cost] I2 --> J2{Race Condition?} J2 -->|Won Race| K2[Use Set Value] J2 -->|Lost Race| L2[DecrementIfExists<br/>on Existing Key] K2 --> M2[Buffer and Return] L2 --> D2 %% Async background process N2[Background Workers] --> O2[Consume Buffer] O2 --> P2[Batch DB Updates] E2 -.-> N2 M2 -.-> N2 end %% Lua Script Detail subgraph LUA["Lua Script: decrementIfExistsScript"] S1[GET key] --> S2{Key exists?} S2 -->|No| S3[Return 0 false] S2 -->|Yes| S4[Check: current ge decrement] S4 -->|No| S5[Return current true<br/>Insufficient Credits] S4 -->|Yes| S6[SET key = current - decrement] S6 --> S7[Return new_value true] end classDef oldSystem fill:#ffebee,stroke:#d32f2f,stroke-width:2px,color:#000 classDef newSystem fill:#e8f5e8,stroke:#388e3c,stroke-width:2px,color:#000 classDef luaScript fill:#fff3e0,stroke:#f57c00,stroke-width:2px,color:#000 classDef problem fill:#fce4ec,stroke:#e91e63,stroke-width:1px,color:#000 class OLD oldSystem; class NEW newSystem; class LUA luaScript; class I1 problem;Type of change
How should this be tested?
Unit tests and manual tests please.
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Performance
Bug Fixes
Tests