fix: reject lossy metering usage values - #1630
Open
kang-heewon wants to merge 2 commits into
Open
Conversation
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (21)
📝 WalkthroughWalkthrough사용량을 1부터 2,147,483,647까지의 정수로 제한하는 검증을 추가했습니다. 잘못된 입력은 idempotency 및 저장 전에 거부되며, Redis의 무효 값·안전 정수 범위 초과·소수 누적은 fail-closed로 처리됩니다. 새 문제 타입과 문서, 테스트도 추가되었습니다. Changes사용량 오류 계약
입력 경계 검증
Redis 누적 및 조회 검증
문서 및 릴리스 메타데이터
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant MeteringService
participant RedisUsageStorage
participant Redis
Client->>MeteringService: record(value)
MeteringService->>MeteringService: validateUsageValue(value)
MeteringService->>RedisUsageStorage: recordUsage(valid value)
RedisUsageStorage->>Redis: idempotency and usage storage
Redis-->>RedisUsageStorage: stored usage members
RedisUsageStorage-->>Client: usage result or Problem error
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
kang-heewon
force-pushed
the
fix/1577-integer-usage-values
branch
from
July 30, 2026 11:22
7eea0ef to
f642b06
Compare
📊 Benchmark Results✅ All benchmarks passed
Updated: 2026-07-31T03:42:25.741Z · Commit: 0799a06 |
kang-heewon
force-pushed
the
fix/1577-integer-usage-values
branch
from
July 30, 2026 11:49
f642b06 to
82eb51a
Compare
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 (1)
packages/metering-core/src/libs/RedisUsageStorage.ts (1)
82-101: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLua 스크립트 내
Number.MAX_SAFE_INTEGER매직 넘버 중복.
9007199254740991이 동일 Lua 스크립트 문자열 안에서 두 번(누적 중간 합,newUsage) 하드코딩되어 있습니다. 로컬 변수로 한 번만 선언해 재사용하면 값이 어긋날 위험을 줄이고 가독성도 개선됩니다.♻️ 제안하는 리팩터링
local usageKey = KEYS[1] local dedupeKey = ${dedupeKeyLiteral} local quota = tonumber(ARGV[1]) local value = tonumber(ARGV[2]) local score = tonumber(ARGV[3]) local member = ARGV[4] local allowOverQuota = ARGV[5] == '1' local ttlSeconds = ${RedisUsageStorage.RECORD_IDEMPOTENCY_TTL_SECONDS} +local maxSafeInteger = 9007199254740991 local records = redis.call('ZRANGEBYSCORE', usageKey, '-inf', '+inf') local currentUsage = 0 for _, existingMember in ipairs(records) do local usageValue = string.match(existingMember, '^[^:]+:([1-9]%d*):') or string.match(existingMember, '^[^:]+:([1-9]%d*)$') local numericUsageValue = usageValue and tonumber(usageValue) if not numericUsageValue or numericUsageValue > ${MAX_USAGE_VALUE} then return redis.error_reply('Invalid stored usage value') end currentUsage = currentUsage + numericUsageValue - if currentUsage > 9007199254740991 then + if currentUsage > maxSafeInteger then return redis.error_reply('Usage total exceeds the safe integer range') end end if redis.call('EXISTS', dedupeKey) == 1 then return { 0, currentUsage } end local newUsage = currentUsage + value -if newUsage > 9007199254740991 then +if newUsage > maxSafeInteger then return redis.error_reply('Usage total exceeds the safe integer range') end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/metering-core/src/libs/RedisUsageStorage.ts` around lines 82 - 101, Define a local Lua variable for the safe integer limit once before the usage-total checks, then replace both 9007199254740991 literals in the existing records accumulation and newUsage validation with that variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/metering-core/src/libs/types.ts`:
- Around line 36-37: Update the documentation comments for both usage amount
fields around value and the corresponding field near lines 53–54 to explicitly
state the supported inclusive range is 1 through 2,147,483,647, matching
validateUsageValue, rather than describing it only as a positive 32-bit integer.
In `@packages/metering-core/src/libs/validateUsageValue.ts`:
- Around line 1-9: Extract MAX_USAGE_VALUE into a dedicated usage-value limits
module, then import and reuse it in both validateUsageValue and
InvalidUsageValueProblem so the validation boundary and default error reason
cannot diverge. Remove the duplicate local constant and literal while preserving
the existing validation behavior.
In `@packages/metering-core/src/tests/MeteringService.spec.ts`:
- Around line 139-148: Update the asynchronous exception assertions in
MeteringService.spec.ts around service.record calls, including the cases near
the existing assertion and lines 216-224, to use rejects.toThrow for
InvalidUsageValueProblem or its message. Preserve the required error code
validation separately rather than using rejects.toMatchObject for the rejection.
---
Outside diff comments:
In `@packages/metering-core/src/libs/RedisUsageStorage.ts`:
- Around line 82-101: Define a local Lua variable for the safe integer limit
once before the usage-total checks, then replace both 9007199254740991 literals
in the existing records accumulation and newUsage validation with that variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7e648da5-ca75-4531-81a6-546fd8357f9b
⛔ Files ignored due to path filters (1)
packages/problems-core/src/generated/problem-code-registry.tsis excluded by!**/generated/**
📒 Files selected for processing (20)
.changeset/strict-metering-usage-values.mddocs/problem-code-registry.jsonpackages/docs/src/content/docs/api/metering-core/src/classes/InvalidUsageValueProblem.mdpackages/docs/src/content/docs/api/metering-core/src/classes/MeteringService.mdpackages/docs/src/content/docs/api/metering-core/src/type-aliases/RecordOptions.mdpackages/docs/src/content/docs/api/metering-core/src/type-aliases/UsageRecord.mdpackages/docs/src/content/docs/api/problems-core/src/classes/Problem.mdpackages/docs/src/content/docs/api/problems-core/src/variables/CROCO_PROBLEM_CODE_REGISTRY.mdpackages/docs/src/content/docs/en/reference/problem-recovery-cookbook.mdpackages/metering-core/README.mdpackages/metering-core/src/index.tspackages/metering-core/src/libs/MeteringService.tspackages/metering-core/src/libs/RedisUsageStorage.tspackages/metering-core/src/libs/problems/InvalidUsageValueProblem.tspackages/metering-core/src/libs/types.tspackages/metering-core/src/libs/validateUsageValue.tspackages/metering-core/src/tests/MeteringService.spec.tspackages/metering-core/src/tests/RedisUsageStorage.spec.tspackages/metering-core/src/tests/problems/Problems.spec.tspublic-api-surface.snapshot.json
kang-heewon
force-pushed
the
fix/1577-integer-usage-values
branch
from
July 30, 2026 12:50
82eb51a to
80a0d34
Compare
Member
Author
|
Addressed the outside-diff Lua review note in 80a0d34: the script now defines maxSafeInteger once from Number.MAX_SAFE_INTEGER and reuses it for both aggregate checks. The focused metering suite (244 tests), repository gate (24/25; one not applicable), and pre-push full test/typecheck (232/232 and 231/231 tasks) pass. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/strict-metering-usage-values.md:
- Around line 1-6: Update the changeset entry for `@croco/metering-core` from
patch to minor to reflect the breaking usage-value validation contract; leave
`@croco/problems-core` at patch unless its public API is also changed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cbed1318-499e-4020-9b5c-447de410b233
⛔ Files ignored due to path filters (1)
packages/problems-core/src/generated/problem-code-registry.tsis excluded by!**/generated/**
📒 Files selected for processing (21)
.changeset/strict-metering-usage-values.mddocs/problem-code-registry.jsonpackages/docs/src/content/docs/api/metering-core/src/classes/InvalidUsageValueProblem.mdpackages/docs/src/content/docs/api/metering-core/src/classes/MeteringService.mdpackages/docs/src/content/docs/api/metering-core/src/type-aliases/RecordOptions.mdpackages/docs/src/content/docs/api/metering-core/src/type-aliases/UsageRecord.mdpackages/docs/src/content/docs/api/problems-core/src/classes/Problem.mdpackages/docs/src/content/docs/api/problems-core/src/variables/CROCO_PROBLEM_CODE_REGISTRY.mdpackages/docs/src/content/docs/en/reference/problem-recovery-cookbook.mdpackages/metering-core/README.mdpackages/metering-core/src/index.tspackages/metering-core/src/libs/MeteringService.tspackages/metering-core/src/libs/RedisUsageStorage.tspackages/metering-core/src/libs/problems/InvalidUsageValueProblem.tspackages/metering-core/src/libs/types.tspackages/metering-core/src/libs/usageValueLimits.tspackages/metering-core/src/libs/validateUsageValue.tspackages/metering-core/src/tests/MeteringService.spec.tspackages/metering-core/src/tests/RedisUsageStorage.spec.tspackages/metering-core/src/tests/problems/Problems.spec.tspublic-api-surface.snapshot.json
kang-heewon
force-pushed
the
fix/1577-integer-usage-values
branch
from
July 30, 2026 13:25
80a0d34 to
c837b60
Compare
kang-heewon
force-pushed
the
fix/1577-integer-usage-values
branch
from
July 30, 2026 13:54
c837b60 to
909ddbd
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/metering-core/src/libs/MeteringService.ts`:
- Around line 60-61: Update both InvalidUsageValueProblem JSDoc blocks in
packages/metering-core/src/libs/MeteringService.ts at lines 60-61 and 72-73 to
state that value must be an integer from 1 through 2,147,483,647. Regenerate the
TypeDoc output so the corresponding entries at
packages/docs/src/content/docs/api/metering-core/src/classes/MeteringService.md
lines 96-99 and 127-130 reflect the source change; do not edit generated
documentation manually, and validate with pnpm docs:api:check and the full
documentation build.
In `@packages/metering-core/src/tests/RedisUsageStorage.spec.ts`:
- Around line 307-322: Update the invalid-value test around storage.record to
verify that Redis is not touched through the actual mockRedis.eval interaction,
rather than mockRedis.set or mockRedis.zadd. Assert eval is not called after the
rejection so the test genuinely covers the pre-idempotency validation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c6bbad0d-ef24-42c9-9b92-880e85257841
⛔ Files ignored due to path filters (1)
packages/problems-core/src/generated/problem-code-registry.tsis excluded by!**/generated/**
📒 Files selected for processing (21)
.changeset/strict-metering-usage-values.mddocs/problem-code-registry.jsonpackages/docs/src/content/docs/api/metering-core/src/classes/InvalidUsageValueProblem.mdpackages/docs/src/content/docs/api/metering-core/src/classes/MeteringService.mdpackages/docs/src/content/docs/api/metering-core/src/type-aliases/RecordOptions.mdpackages/docs/src/content/docs/api/metering-core/src/type-aliases/UsageRecord.mdpackages/docs/src/content/docs/api/problems-core/src/classes/Problem.mdpackages/docs/src/content/docs/api/problems-core/src/variables/CROCO_PROBLEM_CODE_REGISTRY.mdpackages/docs/src/content/docs/en/reference/problem-recovery-cookbook.mdpackages/metering-core/README.mdpackages/metering-core/src/index.tspackages/metering-core/src/libs/MeteringService.tspackages/metering-core/src/libs/RedisUsageStorage.tspackages/metering-core/src/libs/problems/InvalidUsageValueProblem.tspackages/metering-core/src/libs/types.tspackages/metering-core/src/libs/usageValueLimits.tspackages/metering-core/src/libs/validateUsageValue.tspackages/metering-core/src/tests/MeteringService.spec.tspackages/metering-core/src/tests/RedisUsageStorage.spec.tspackages/metering-core/src/tests/problems/Problems.spec.tspublic-api-surface.snapshot.json
kang-heewon
force-pushed
the
fix/1577-integer-usage-values
branch
3 times, most recently
from
July 30, 2026 17:48
d774d76 to
7e3f18f
Compare
kang-heewon
force-pushed
the
fix/1577-integer-usage-values
branch
from
July 31, 2026 03:32
7e3f18f to
bea6c31
Compare
Member
Author
|
The required |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Outcome
Metering now accepts only integer usage values from 1 through 2,147,483,647, matching every supported storage adapter. Fractional, non-finite, non-positive, and oversized values fail with
metering/invalid-usage-valuebefore registry lookup, idempotency, or storage.Redis aggregation, record fetching, and atomic quota checks now reject malformed legacy members instead of truncating them. The TypeScript parser and Lua quota script share the same maximum and fail closed on unsafe aggregate totals.
Verification
pnpm --filter @croco/metering-core test— 255 tests passed, 2 opt-in Redis integration tests skippedpnpm --filter @croco/metering-core typecheckpnpm --filter @croco/metering-core lintpnpm --filter @croco/metering-core buildpnpm public-api:checkpnpm problem-registry:checkpnpm docs:api:checkpnpm check— 24/25 passed, 1 not applicableReview gates
INTEGERResidual risk
The unit suite mocks
EVAL; the actual Lua path was additionally exercised against local Redis 8.8. The repository now also includes opt-in Redis integration coverage for idempotency; malformed-member fail-closed behavior was exercised by the Redis 8.8 smoke above.Fixes #1577
Summary by CodeRabbit
InvalidUsageValueProblem(metering/invalid-usage-value, HTTP 422)로 거부합니다.