fix: use Decimal.js for cost calculations - #1193
Conversation
Prevent floating point rounding errors in cost calculations throughout the system by using decimal.js in both gateway cost calculation and worker log processing. Ensures costs like 0.0043880400000000002 are properly calculated and accumulated without precision loss.
WalkthroughIntroduces Decimal.js library across gateway and worker services to replace floating-point arithmetic with precise decimal calculations. Converts cost computations in the gateway to use Decimal objects and updates worker cost-tracking maps from numeric to Decimal-based values. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/worker/src/worker.ts (1)
536-619: Decimal-based aggregation looks correct; consider string params for DB decimalsThe switch to
Map<string, Decimal>fororgCosts/apiKeyCosts,plus(...)accumulation, andgreaterThan(0)checks is logically correct and solves the intra‑batch floating‑point drift that caused weird cost logs. The control flow and gating (row.cost && row.cost > 0 && !row.cached,used_mode === "credits") are preserved.One refinement to consider, given
organization.creditsandapiKey.usageare decimal columns:
Instead of:
const costNumber = totalCost.toNumber(); credits: sql`${organization.credits} - ${costNumber}`, usage: sql`${apiKey.usage} + ${costNumber}`,use the decimal string to avoid any final IEEE‑754 conversion when sending parameters to Postgres:
const costString = totalCost.toString(); credits: sql`${organization.credits} - ${costString}::numeric`, usage: sql`${apiKey.usage} + ${costString}::numeric`,This keeps the entire path (calculation + DB update) in exact decimal form. Please verify with your Drizzle/PG setup that passing strings with an explicit
::numericcast behaves as expected.apps/gateway/src/lib/costs.ts (1)
1-1: Decimal.js integration preserves cost semantics and fixes FP driftThe switch to Decimal for
inputPrice/outputPrice/cachedInputPrice/requestPrice, thediscountMultipliervianew Decimal(1).minus(discount), and the cost formulas (inputCost,outputCost,cachedInputCost,requestCost,totalCost) all preserve the previous logic while removing floating‑point accumulation issues. Returning*.toNumber()keeps the external contract unchanged.Two small follow‑ups to consider:
Import style – As in the worker, please confirm
{ Decimal }is a valid import form for your decimal.js v10.5.0 ESM/TS setup; otherwise switch toimport Decimal from "decimal.js";here as well.If you ever need fully exact external values – Right now you re‑enter JS Number space at the edge. If in future you want to expose exact decimal strings (e.g., for high‑precision billing), you could return
inputCost.toString()etc. instead of numbers and adjust callers/DB schema accordingly. Not necessary for this PR, but worth noting.Also applies to: 172-213
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (4)
apps/gateway/package.json(1 hunks)apps/gateway/src/lib/costs.ts(3 hunks)apps/worker/package.json(1 hunks)apps/worker/src/worker.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/worker/src/worker.ts (1)
packages/db/src/schema.ts (2)
organization(108-145)apiKey(238-265)
🪛 ESLint
apps/worker/src/worker.ts
[error] 1-1: Resolve error: EACCES: permission denied, open '/zujfnxWGQn'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/.pnpm/eslint-import-resolver-typescript@4.4.4_eslint-plugin-import@2.32.0_eslint@9.34.0_jiti@2.6.1_/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.6.1__t_eb0afb446ca3f59399f5d681f0059e64/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.6.1__t_eb0afb446ca3f59399f5d681f0059e64/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.6.1__t_eb0afb446ca3f59399f5d681f0059e64/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.6.1__t_eb0afb446ca3f59399f5d681f0059e64/node_modules/eslint-module-utils/resolve.js:233:12)
at resolveImportType (/home/jailuser/git/node_modules/.pnpm/eslint-plugin-import@2.32.0_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.6.1___986ec7d736a20dae59d4d473ff8a6f0d/node_modules/eslint-plugin-import/lib/core/importType.js:126:2822)
at computeRank (/home/jailuser/git/node_modules/.pnpm/eslint-plugin-import@2.32.0_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.6.1___986ec7d736a20dae59d4d473ff8a6f0d/node_modules/eslint-plugin-import/lib/rules/order.js:529:43)
(import/order)
[error] 1-1: Resolve error: EACCES: permission denied, open '/oWnYEzcjIf'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/.pnpm/eslint-import-resolver-typescript@4.4.4_eslint-plugin-import@2.32.0_eslint@9.34.0_jiti@2.6.1_/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.6.1__t_eb0afb446ca3f59399f5d681f0059e64/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.6.1__t_eb0afb446ca3f59399f5d681f0059e64/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.6.1__t_eb0afb446ca3f59399f5d681f0059e64/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.6.1__t_eb0afb446ca3f59399f5d681f0059e64/node_modules/eslint-module-utils/resolve.js:233:12)
at checkSourceValue (/home/jailuser/git/node_modules/.pnpm/eslint-plugin-import@2.32.0_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.6.1___986ec7d736a20dae59d4d473ff8a6f0d/node_modules/eslint-plugin-import/lib/rules/no-useless-path-segments.js:85:53)
at checkSourceValue (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.6.1__t_eb0afb446ca3f59399f5d681f0059e64/node_modules/eslint-module-utils/moduleVisitor.js:32:5)
(import/no-useless-path-segments)
apps/gateway/src/lib/costs.ts
[error] 1-1: Resolve error: EACCES: permission denied, open '/oYgRVpcYic'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/.pnpm/get-tsconfig@4.10.1/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/.pnpm/eslint-import-resolver-typescript@4.4.4_eslint-plugin-import@2.32.0_eslint@9.34.0_jiti@2.6.1_/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.6.1__t_eb0afb446ca3f59399f5d681f0059e64/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.6.1__t_eb0afb446ca3f59399f5d681f0059e64/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.6.1__t_eb0afb446ca3f59399f5d681f0059e64/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/.pnpm/eslint-module-utils@2.12.1_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.6.1__t_eb0afb446ca3f59399f5d681f0059e64/node_modules/eslint-module-utils/resolve.js:233:12)
at resolveImportType (/home/jailuser/git/node_modules/.pnpm/eslint-plugin-import@2.32.0_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.6.1___986ec7d736a20dae59d4d473ff8a6f0d/node_modules/eslint-plugin-import/lib/core/importType.js:126:2822)
at computeRank (/home/jailuser/git/node_modules/.pnpm/eslint-plugin-import@2.32.0_@typescript-eslint+parser@8.39.1_eslint@9.34.0_jiti@2.6.1___986ec7d736a20dae59d4d473ff8a6f0d/node_modules/eslint-plugin-import/lib/rules/order.js:529:43)
(import/order)
⏰ 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). (17)
- GitHub Check: build-split (docs, linux/amd64)
- GitHub Check: build-split (worker, linux/amd64)
- GitHub Check: build-split (gateway, linux/amd64)
- GitHub Check: build-split (ui, linux/amd64)
- GitHub Check: build-split (playground, linux/amd64)
- GitHub Check: build-split (api, linux/amd64)
- GitHub Check: build-unified (linux/amd64)
- GitHub Check: e2e-shards (1)
- GitHub Check: e2e-shards (5)
- GitHub Check: e2e-shards (2)
- GitHub Check: test / run
- GitHub Check: e2e-shards (4)
- GitHub Check: build / run
- GitHub Check: e2e-shards (3)
- GitHub Check: generate / run
- GitHub Check: lint / run
- GitHub Check: autofix
🔇 Additional comments (3)
apps/worker/package.json (1)
25-34: Decimal.js dependency addition looks consistentAdding
"decimal.js": "10.5.0"aligns with the rest of the pinned dependencies and the new Decimal‑based cost logic in the worker.Please double‑check that all packages in the repo that use Decimal.js are on the same version to avoid subtle behavior differences.
apps/gateway/package.json (1)
25-48: Gateway decimal.js dependency matches workerIncluding
"decimal.js": "10.5.0"here keeps gateway and worker in sync for Decimal‑based cost calculations.Please confirm there are no other places in the monorepo pulling in a different Decimal.js version (e.g., via transitive deps) that might cause multiple copies at runtime.
apps/worker/src/worker.ts (1)
1-1: Confirm Decimal.js import styleYou’re importing Decimal via a named import (
{ Decimal }), whereas many examples use the default export (import Decimal from "decimal.js"). Whether this is correct depends on how decimal.js publishes its ESM/TS typings in your toolchain.Please confirm with your TS/compiler setup (or decimal.js typings) that named import
{ Decimal }is valid here; if not, switch toimport Decimal from "decimal.js";in both this file andapps/ggateway/src/lib/costs.ts.
1 participant
Summary
Prevents floating point rounding errors in cost calculations by using decimal.js throughout the system. This fixes issues like costs being logged as 0.0043880400000000002 instead of the expected precise value.
Changes
Summary by CodeRabbit
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.