fix(api): cap unbounded async concurrency with p-limit#1975
Open
anis-marrouchi wants to merge 2 commits into
Open
fix(api): cap unbounded async concurrency with p-limit#1975anis-marrouchi wants to merge 2 commits into
anis-marrouchi wants to merge 2 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds p-limit-based concurrency caps across several API hotspots so previously unbounded Promise.all fan-outs (sockets, DB checks, attachment storage, translation lookups, filesystem traversal, workflow memory updates) now run with bounded concurrency; request/response shapes and authorization logic remain unchanged. ChangesConcurrency Limits (p-limit)
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Limiter as "p-limit (N)"
participant DB
participant FS
participant Storage
participant Socket
Client->>API: Request triggers batched async work
API->>Limiter: enqueue tasks (auth, joins, FS reads, stores, translations)
Limiter--)API: allow up to N concurrent tasks
API->>DB: permission checks / translation lookups (bounded)
API->>FS: extension dir/file reads (bounded)
API->>Storage: store attachments (bounded)
API->>Socket: join rooms (bounded)
DB-->>API: DB responses
FS-->>API: file lists
Storage-->>API: store confirmations
Socket-->>API: join confirmations
API-->>Client: aggregate results (same payload shape)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/api/src/app.service.ts (1)
84-84: ⚡ Quick winUse a named constant (with a short rationale) for the room-join cap.
The inline
15is a tuning knob; extracting it (e.g.,SOCKET_JOIN_CONCURRENCY) with a one-line comment will make future tuning safer and clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/app.service.ts` at line 84, Extract the magic number 15 into a named constant (e.g., SOCKET_JOIN_CONCURRENCY) and replace the inline literal when creating socketLimit (pLimit). Add a one-line comment explaining this is the concurrency cap for joining rooms so it’s easy to tune later; update the usage in the socketLimit assignment (where pLimit(15) is called) to pLimit(SOCKET_JOIN_CONCURRENCY).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/api/src/i18n/loaders/extension-json.loader.ts`:
- Around line 190-203: The current findI18nDirectories function creates a new
pLimit(8) (fsLimit) on each recursive call which allows uncontrolled total
concurrency; refactor so the limiter is created once and reused across
recursion—either add an optional parameter (e.g., fsLimit: ReturnType<typeof
pLimit>) to findI18nDirectories and pass the same limiter into recursive calls,
or make a shared class/module-level limiter used by findI18nDirectories; replace
the local pLimit(8) usage with the shared limiter wherever entries are mapped to
limiters so total concurrent readdir/stat operations stay globally capped at 8.
---
Nitpick comments:
In `@packages/api/src/app.service.ts`:
- Line 84: Extract the magic number 15 into a named constant (e.g.,
SOCKET_JOIN_CONCURRENCY) and replace the inline literal when creating
socketLimit (pLimit). Add a one-line comment explaining this is the concurrency
cap for joining rooms so it’s easy to tune later; update the usage in the
socketLimit assignment (where pLimit(15) is called) to
pLimit(SOCKET_JOIN_CONCURRENCY).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d690c50-1abe-412b-8857-b9a73fd0b7da
📒 Files selected for processing (6)
packages/api/src/app.service.tspackages/api/src/attachment/guards/attachment-ability.guard.tspackages/api/src/channel/lib/Handler.tspackages/api/src/i18n/controllers/translation.controller.tspackages/api/src/i18n/loaders/extension-json.loader.tspackages/api/src/workflow/utils/memory-store.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/api/src/i18n/loaders/extension-json.loader.ts (1)
162-163: ⚡ Quick winExtract and document the concurrency cap
Line 162 uses a bare
8. Please extract it to a named constant with a short tuning note to make future adjustments explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/i18n/loaders/extension-json.loader.ts` around lines 162 - 163, Extract the hard-coded concurrency cap (the literal 8 passed to pLimit when creating fsLimit) into a named constant (e.g., FILE_READ_CONCURRENCY = 8) and replace the magic number in the fsLimit = pLimit(…) call with that constant; add a short inline comment above the constant explaining why the cap exists and a tuning note for future adjustments (e.g., "tune based on IO throughput / system limits"). Ensure references to fsLimit and pLimit remain unchanged elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/api/src/i18n/loaders/extension-json.loader.ts`:
- Around line 166-167: The current implementation acquires the same fsLimit
semaphore at both the parent mapping of extensionRootPaths and inside recursive
findI18nDirectories, which can deadlock when many parents occupy all slots
waiting on children; change the concurrency strategy by removing the outer
fsLimit wrapper around the recursive call sites and instead centralize semaphore
usage into low-level filesystem helpers (wrap fs.readdir/fs.stat/etc. with a
helper that calls fsLimit.run or similar) so findI18nDirectories and any callers
call those helpers without re-acquiring the semaphore; update references in the
code (fsLimit, findI18nDirectories, extensionRootPaths mapping) to use the new
fs-limited helpers for I/O only, keeping recursion itself semaphore-free.
---
Nitpick comments:
In `@packages/api/src/i18n/loaders/extension-json.loader.ts`:
- Around line 162-163: Extract the hard-coded concurrency cap (the literal 8
passed to pLimit when creating fsLimit) into a named constant (e.g.,
FILE_READ_CONCURRENCY = 8) and replace the magic number in the fsLimit =
pLimit(…) call with that constant; add a short inline comment above the constant
explaining why the cap exists and a tuning note for future adjustments (e.g.,
"tune based on IO throughput / system limits"). Ensure references to fsLimit and
pLimit remain unchanged elsewhere.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e6b45d8-75e5-483e-89ad-7bc89b76142d
📒 Files selected for processing (1)
packages/api/src/i18n/loaders/extension-json.loader.ts
Comment on lines
+166
to
167
| fsLimit(() => this.findI18nDirectories(rootPath, fsLimit)), | ||
| ), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify nested limiter acquisition pattern in the changed file.
# Expected: matches both (1) top-level limited recursive call and (2) recursive limited child call.
fd -i 'extension-json.loader.ts' --exec rg -n -C2 \
'fsLimit\(\(\) => this\.findI18nDirectories\(rootPath, fsLimit\)\)|fsLimit\(async \(\) =>|return this\.findI18nDirectories\(entryPath, fsLimit\)'Repository: Hexastack/Hexabot
Length of output: 599
🏁 Script executed:
sed -n '155,210p' packages/api/src/i18n/loaders/extension-json.loader.tsRepository: Hexastack/Hexabot
Length of output: 1467
🏁 Script executed:
rg -n 'resolveExtensionTranslationFiles' packages/api/src/i18n/loaders/extension-json.loader.tsRepository: Hexastack/Hexabot
Length of output: 191
🏁 Script executed:
sed -n '110,130p' packages/api/src/i18n/loaders/extension-json.loader.tsRepository: Hexastack/Hexabot
Length of output: 775
🏁 Script executed:
rg -n 'extensionPaths' packages/api/src/i18n/loaders/extension-json.loader.tsRepository: Hexastack/Hexabot
Length of output: 344
🏁 Script executed:
# Check how this loader is instantiated and what paths are typically provided
rg -n 'ExtensionJsonLoader' packages/api/src --type ts -A 3 | head -40Repository: Hexastack/Hexabot
Length of output: 3201
Nested fsLimit acquisition in recursive traversal creates deadlock risk
The pattern at lines 166 and 197–204 acquires the same fsLimit semaphore across parent and child tasks. When extensionRootPaths has many entries or subdirectories are numerous, all 8 slots can be consumed by parent tasks waiting on children that need the same slots, blocking progress.
Consider limiting filesystem operations (e.g., fs.readdir) directly instead of wrapping entire recursive frames. This isolates I/O concurrency control from recursive call stack, preventing nested slot acquisition:
Suggested refactor
private async resolveExtensionTranslationFiles(
extensionRootPaths: string[],
): Promise<string[]> {
const fsLimit = pLimit(8);
const i18nDirectories = (
await Promise.all(
- extensionRootPaths.map((rootPath) =>
- fsLimit(() => this.findI18nDirectories(rootPath, fsLimit)),
- ),
+ extensionRootPaths.map((rootPath) =>
+ this.findI18nDirectories(rootPath, fsLimit),
+ ),
)
).flat();
// ... rest unchanged
}
private async findI18nDirectories(
rootPath: string,
fsLimit: ReturnType<typeof pLimit>,
): Promise<string[]> {
const entries = await fsLimit(() =>
fs.readdir(rootPath, { withFileTypes: true }).catch(() => null),
);
if (!entries) {
return [];
}
const nestedResults = await Promise.all(
entries
.filter((entry) => entry.isDirectory())
.map((entry) => {
const entryPath = path.join(rootPath, entry.name);
if (entry.name === 'i18n') {
return Promise.resolve([entryPath]);
}
return this.findI18nDirectories(entryPath, fsLimit);
}),
);
return nestedResults.flat();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/src/i18n/loaders/extension-json.loader.ts` around lines 166 -
167, The current implementation acquires the same fsLimit semaphore at both the
parent mapping of extensionRootPaths and inside recursive findI18nDirectories,
which can deadlock when many parents occupy all slots waiting on children;
change the concurrency strategy by removing the outer fsLimit wrapper around the
recursive call sites and instead centralize semaphore usage into low-level
filesystem helpers (wrap fs.readdir/fs.stat/etc. with a helper that calls
fsLimit.run or similar) so findI18nDirectories and any callers call those
helpers without re-acquiring the semaphore; update references in the code
(fsLimit, findI18nDirectories, extensionRootPaths mapping) to use the new
fs-limited helpers for I/O only, keeping recursion itself semaphore-free.
Replace unbounded Promise.all fan-out with p-limit in 6 hotspots to prevent burst load on DB/filesystem/socket backends: - extension-json.loader: filesystem traversal (cap=8) - translation.controller: DB upserts during refresh (cap=10) - Handler: attachment storage (cap=4) - attachment-ability.guard: permission checks (cap=10) - memory-store: memory entry persistence (cap=8) - app.service: socket room joins (cap=15) Closes #4 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tories Each recursive call was creating a new pLimit(8), defeating the global concurrency cap. Pass the limiter as a parameter so all recursion levels share one queue. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
anis-marrouchi
force-pushed
the
4-cap-unbounded-async-concurrency
branch
from
3b92c1d to
7885d33
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/api/src/attachment/guards/attachment-ability.guard.ts`:
- Around line 250-258: You are creating per-call pLimit instances so DB checks
still fan out; lift the concurrency limiter to the request scope and use the
same limiter for all permission checks: create one pLimit(10) (the existing
dbLimit) and stop isAuthorized from constructing its own pLimit per
invocation—either add a limiter parameter to isAuthorized and pass dbLimit
through (from the resourceRef.map branch) or move the DB-limited work into a
helper that accepts the shared dbLimit; ensure all calls that previously created
pLimit inside isAuthorized (and any callers) use this single shared dbLimit so
all resourceRef checks and nested DB operations share the same concurrency cap.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97ec7985-a0eb-42ec-94e8-0373f66f23d9
📒 Files selected for processing (6)
packages/api/src/app.service.tspackages/api/src/attachment/guards/attachment-ability.guard.tspackages/api/src/channel/lib/Handler.tspackages/api/src/i18n/controllers/translation.controller.tspackages/api/src/i18n/loaders/extension-json.loader.tspackages/api/src/workflow/utils/memory-store.ts
✅ Files skipped from review due to trivial changes (2)
- packages/api/src/app.service.ts
- packages/api/src/i18n/loaders/extension-json.loader.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/api/src/workflow/utils/memory-store.ts
Comment on lines
+250
to
258
| const dbLimit = pLimit(10); | ||
|
|
||
| return ( | ||
| await Promise.all( | ||
| resourceRef.map((c) => this.isAuthorized(Action.READ, user, c)), | ||
| resourceRef.map((c) => | ||
| dbLimit(() => this.isAuthorized(Action.READ, user, c)), | ||
| ), | ||
| ) | ||
| ).every(Boolean); |
Contributor
There was a problem hiding this comment.
Share one concurrency budget across the whole GET authorization flow.
isAuthorized() now creates a fresh pLimit(10) per call, and this branch adds another pLimit(10) around each resourceRef. With multiple refs in a single request, the permission checks still fan out per ref instead of sharing one cap, so the DB burst can remain much higher than intended. Please thread one limiter through both loops or lift it to the request scope.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/api/src/attachment/guards/attachment-ability.guard.ts` around lines
250 - 258, You are creating per-call pLimit instances so DB checks still fan
out; lift the concurrency limiter to the request scope and use the same limiter
for all permission checks: create one pLimit(10) (the existing dbLimit) and stop
isAuthorized from constructing its own pLimit per invocation—either add a
limiter parameter to isAuthorized and pass dbLimit through (from the
resourceRef.map branch) or move the DB-limited work into a helper that accepts
the shared dbLimit; ensure all calls that previously created pLimit inside
isAuthorized (and any callers) use this single shared dbLimit so all resourceRef
checks and nested DB operations share the same concurrency cap.
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.
What changed?
Summary: Replaced unbounded
Promise.all(array.map(async ...))fan-out withp-limitin 6 API hotspots where input size is user/config-driven.Why: Unbounded async loops can spike DB/socket/filesystem pressure under large payloads, causing event-loop contention and degraded tail latency.
p-limit(already a dependency) caps concurrency with sensible per-domain defaults.Hotspots capped:
extension-json.loader.tstranslation.controller.tsHandler.tsattachment-ability.guard.tsmemory-store.tsapp.service.tsHow to review: Each file has a small, isolated change —
pLimit(N)wrapping the existing.map()callback. No behavior or response payload changes.Validation: Typecheck and lint pass. Existing behavior preserved — only execution concurrency is bounded.
Closes anis-marrouchi#4
Related upstream issue: #1863
🤖 Generated with Claude Code
Summary by CodeRabbit