Improve performance for pools with 30k+ users#38
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRemoved react-hooks ESLint rules, expanded README install and cronjob instructions, added NodeCache-based caching and new top-user DB queries in the API, increased DB pool sizes and introduced an exported async getDb() accessor, and bumped several dependencies (added node-cache). Changes
Sequence DiagramsequenceDiagram
participant Client
participant API as API Layer
participant Cache as NodeCache
participant DB as Database
Client->>API: getLatestPoolStats()
API->>Cache: get("pool:latest")
alt Cache Hit
Cache-->>API: cached stats
else Cache Miss
API->>DB: query latest pool stats
DB-->>API: stats
API->>Cache: set("pool:latest", stats, 60)
end
API-->>Client: stats
Client->>API: getTopUserHashrates(limit)
API->>Cache: get("top:hashrates:" + limit)
alt Cache Hit
Cache-->>API: cached rankings
else Cache Miss
API->>DB: lateral join latest UserStats per user, order by hashrate1hr, limit
DB-->>API: top users
API->>Cache: set("top:hashrates:" + limit, results, 300)
end
API-->>Client: rankings
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@lib/api.ts`:
- Around line 182-190: The map that builds `result` in lib/api.ts calls
`.toString()` on fields like `bestEver`, `hashrate1hr`, `hashrate1d`,
`hashrate7d`, and `bestShare` which will throw if any are null; update the
mapping in the function that returns `result` to defensively handle
null/undefined (e.g., use nullish coalescing or String(value ?? 0) or
value?.toString() ?? "0") so you never call `.toString()` on null, and apply the
same defensive pattern to the analogous mapping in getTopUserHashrates to
prevent runtime errors from NULL DB columns.
🧹 Nitpick comments (5)
lib/db.ts (2)
26-27: Consider the aggregate connection count across Next.js workers.With
min: 10, each Node.js process will eagerly open 10 connections. If Next.js spawns multiple workers (common in production), the total minimum connections could exceed PostgreSQL'smax_connectionsdefault (typically 100). Ensure your PostgreSQLmax_connectionsaccommodatesmin * number_of_workersplus headroom for scripts (seed,update-users,cleanup) that also connect.
56-73: Race-condition comment is accurate for Node.js; minor nit on identity.then().The concurrency guard is correct — Node.js is single-threaded so the
if (!connectionPromise)check plus immediate assignment is atomic. The.then((connection) => { return connection; })on line 61-62 is a no-op identity transform that can be simplified.♻️ Suggested simplification
if (!connectionPromise) { - connectionPromise = AppDataSource.initialize() - .then((connection) => { - return connection; - }) - .catch((error) => { + connectionPromise = AppDataSource.initialize().catch((error) => { console.error('Database connection error:', error); connectionPromise = null; throw error; }); }package.json (1)
26-26:nextchanged from pinned to caret range.The version specifier changed from
"14.2.18"(exact) to"^14.2.18"(caret). This now allows automatic minor/patch upgrades within 14.x. If the previous pinning was intentional (e.g., to avoid regressions), consider keeping it exact or using a tilde~14.2.18for patch-only updates.lib/api.ts (2)
154-158: Inconsistent cache-hit check — use!== undefinedfor safety.
getLatestPoolStats(line 31) andgetHistoricalPoolStats(line 54) correctly usecached !== undefinedto distinguish a cache miss from a cached falsy value. Here and ingetTopUserHashrates(line 214), you useif (cached), which would skip the cache if the cached value were an empty array[]… except empty arrays are truthy in JS, so it works today. Still, aligning with the!== undefinedpattern used elsewhere avoids a subtle trap if the return type ever changes.♻️ Suggested fix
const cached = cache.get(cacheKey); - if (cached) { + if (cached !== undefined) { return cached; }Apply the same change in
getTopUserHashrates(line 214).
113-137:getWorkerWithStatsloads all stats eagerly — consider the same "latest only" optimization.Other functions were refactored to fetch only the latest stat row. This function still loads all worker stats via
relations: { stats: true }and sorts them in-memory. For workers with long histories, this could be a significant amount of data. If callers only need recent stats, consider applying a similar query-builder approach with a limit.
| const result = rows.map((r: any) => ({ | ||
| address: r.userAddress, | ||
| workerCount: r.workerCount, | ||
| difficulty: r.bestEver.toString(), | ||
| hashrate1hr: r.hashrate1hr.toString(), | ||
| hashrate1d: r.hashrate1d.toString(), | ||
| hashrate7d: r.hashrate7d.toString(), | ||
| bestShare: r.bestShare.toString(), | ||
| })); |
There was a problem hiding this comment.
Null safety: .toString() will throw if any field is null.
The raw query returns untyped rows. If any column (bestEver, hashrate1hr, etc.) contains a NULL value, calling .toString() on it will throw a runtime error. While the JOIN LATERAL ensures a stats row exists, individual columns could still be NULL depending on schema constraints.
🛡️ Defensive approach
const result = rows.map((r: any) => ({
address: r.userAddress,
workerCount: r.workerCount,
- difficulty: r.bestEver.toString(),
- hashrate1hr: r.hashrate1hr.toString(),
- hashrate1d: r.hashrate1d.toString(),
- hashrate7d: r.hashrate7d.toString(),
- bestShare: r.bestShare.toString(),
+ difficulty: r.bestEver?.toString() ?? '0',
+ hashrate1hr: r.hashrate1hr?.toString() ?? '0',
+ hashrate1d: r.hashrate1d?.toString() ?? '0',
+ hashrate7d: r.hashrate7d?.toString() ?? '0',
+ bestShare: r.bestShare?.toString() ?? '0',
}));Apply the same pattern to getTopUserHashrates (lines 240-248).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const result = rows.map((r: any) => ({ | |
| address: r.userAddress, | |
| workerCount: r.workerCount, | |
| difficulty: r.bestEver.toString(), | |
| hashrate1hr: r.hashrate1hr.toString(), | |
| hashrate1d: r.hashrate1d.toString(), | |
| hashrate7d: r.hashrate7d.toString(), | |
| bestShare: r.bestShare.toString(), | |
| })); | |
| const result = rows.map((r: any) => ({ | |
| address: r.userAddress, | |
| workerCount: r.workerCount, | |
| difficulty: r.bestEver?.toString() ?? '0', | |
| hashrate1hr: r.hashrate1hr?.toString() ?? '0', | |
| hashrate1d: r.hashrate1d?.toString() ?? '0', | |
| hashrate7d: r.hashrate7d?.toString() ?? '0', | |
| bestShare: r.bestShare?.toString() ?? '0', | |
| })); |
🤖 Prompt for AI Agents
In `@lib/api.ts` around lines 182 - 190, The map that builds `result` in
lib/api.ts calls `.toString()` on fields like `bestEver`, `hashrate1hr`,
`hashrate1d`, `hashrate7d`, and `bestShare` which will throw if any are null;
update the mapping in the function that returns `result` to defensively handle
null/undefined (e.g., use nullish coalescing or String(value ?? 0) or
value?.toString() ?? "0") so you never call `.toString()` on null, and apply the
same defensive pattern to the analogous mapping in getTopUserHashrates to
prevent runtime errors from NULL DB columns.
|
@pdath i think pnpm-lock just needs to be updated and then can merge |
This should be done now. |
2 participants
This branch significantly improves ckstats performance when a pool has a large number of users. CPU load on the PostgreSQL server has been significantly reduced. Key changes include rewriting many queries and introducing node-cache to cache expensive operations.
Also, many modules have been updated, which has reduced the number of "pnpm audit" issues reported in issue #31. Versions have been selected that retain compatibility with Ubuntu 22.04.
A race condition in getDb() has also been resolved.
pnpm is not correctly updating dependencies when you apply this branch. Consequently, you'll need to run "pnpm store prune" before "pnpm build" to ensure it builds correctly.
Summary by CodeRabbit
Documentation
New Features
Improvements
Chores