fix(agents): eliminate EMFILE from SSE tick scanning all meta.json files - #161
Open
hilash wants to merge 1 commit into
Open
fix(agents): eliminate EMFILE from SSE tick scanning all meta.json files#161hilash wants to merge 1 commit into
hilash wants to merge 1 commit into
Conversation
Running counts were re-read from disk on every 3-second SSE tick by opening every meta.json across every cabinet concurrently. Under multiple open cabinets (rooms) the burst of parallel opens exhausted the OS fd limit (EMFILE, os error 24), which also starved turbopack. Two fixes: - getRunningConversationCounts now keeps an in-memory Map bootstrapped once at startup; enqueueConversationNotification delta-updates it on every status transition so no disk reads are needed on subsequent ticks. - listConversationMetas now uses mapCapped (4 cabinets × 20 entries) to bound concurrent file opens during the bootstrap scan and other callers.
📝 WalkthroughWalkthrough
ChangesRunning counts cache and concurrency cap
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/lib/agents/conversation-store.ts`:
- Around line 175-176: The conditional check at line 175 only decrements the
running conversation count for "completed" or "failed" statuses, but the
"cancelled" status should also be treated as a terminal state that decrements
the counter. Modify the condition that checks notification.status to include
"cancelled" alongside "completed" and "failed" so that when a conversation is
cancelled, the _runningCounts.get(slug) value is properly decremented and the
badge count stays accurate.
- Around line 1818-1824: The bootstrap logic guarded by
_runningCountsBootstrapped has a race condition where multiple concurrent
callers can all observe the flag as false and execute the bootstrap
concurrently, causing duplicate increments in _runningCounts. Fix this by
introducing a shared in-flight promise variable (e.g.,
_runningCountsBootstrapPromise) that is set before the bootstrap begins. Check
if this promise exists and await it if it does; otherwise, create the promise,
execute the bootstrap logic that fetches running conversations and populates
_runningCounts, and then mark _runningCountsBootstrapped as true. This ensures
only one bootstrap execution occurs while concurrent callers wait for the same
promise to resolve.
🪄 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: 8dc50828-f19c-47c3-a045-49e33d7d63cf
📒 Files selected for processing (1)
src/lib/agents/conversation-store.ts
Comment on lines
+175
to
+176
| } else if (notification.status === "completed" || notification.status === "failed") { | ||
| const cur = _runningCounts.get(slug) ?? 0; |
There was a problem hiding this comment.
Handle cancelled as a terminal decrement state.
Line 175 only decrements for "completed"/"failed". If a running conversation transitions to "cancelled", the cached running count never drops and badges stay stale.
Suggested fix
- } else if (notification.status === "completed" || notification.status === "failed") {
+ } else if (
+ notification.status === "completed" ||
+ notification.status === "failed" ||
+ notification.status === "cancelled"
+ ) {
const cur = _runningCounts.get(slug) ?? 0;
if (cur <= 1) _runningCounts.delete(slug);
else _runningCounts.set(slug, cur - 1);
}📝 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.
Suggested change
| } else if (notification.status === "completed" || notification.status === "failed") { | |
| const cur = _runningCounts.get(slug) ?? 0; | |
| } else if ( | |
| notification.status === "completed" || | |
| notification.status === "failed" || | |
| notification.status === "cancelled" | |
| ) { | |
| const cur = _runningCounts.get(slug) ?? 0; | |
| if (cur <= 1) _runningCounts.delete(slug); | |
| else _runningCounts.set(slug, cur - 1); | |
| } |
🤖 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 `@src/lib/agents/conversation-store.ts` around lines 175 - 176, The conditional
check at line 175 only decrements the running conversation count for "completed"
or "failed" statuses, but the "cancelled" status should also be treated as a
terminal state that decrements the counter. Modify the condition that checks
notification.status to include "cancelled" alongside "completed" and "failed" so
that when a conversation is cancelled, the _runningCounts.get(slug) value is
properly decremented and the badge count stays accurate.
Comment on lines
+1818
to
+1824
| if (!_runningCountsBootstrapped) { | ||
| const running = await listConversationMetas({ status: "running", limit: 1000 }); | ||
| for (const m of running) { | ||
| _runningCounts.set(m.agentSlug, (_runningCounts.get(m.agentSlug) ?? 0) + 1); | ||
| } | ||
| _runningCountsBootstrapped = true; | ||
| } |
There was a problem hiding this comment.
Guard bootstrap with a shared in-flight promise to prevent double-counting.
Line 1818 has a race: concurrent callers can all observe false, run bootstrap, and each increment _runningCounts, inflating counts.
Suggested fix
const _runningCounts = new Map<string, number>();
let _runningCountsBootstrapped = false;
+let _runningCountsBootstrapPromise: Promise<void> | null = null;
export async function getRunningConversationCounts(): Promise<Record<string, number>> {
if (!_runningCountsBootstrapped) {
- const running = await listConversationMetas({ status: "running", limit: 1000 });
- for (const m of running) {
- _runningCounts.set(m.agentSlug, (_runningCounts.get(m.agentSlug) ?? 0) + 1);
- }
- _runningCountsBootstrapped = true;
+ _runningCountsBootstrapPromise ??= (async () => {
+ const running = await listConversationMetas({ status: "running", limit: 1000 });
+ _runningCounts.clear();
+ for (const m of running) {
+ _runningCounts.set(m.agentSlug, (_runningCounts.get(m.agentSlug) ?? 0) + 1);
+ }
+ _runningCountsBootstrapped = true;
+ })();
+ await _runningCountsBootstrapPromise;
}
return Object.fromEntries(_runningCounts);
}📝 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.
Suggested change
| if (!_runningCountsBootstrapped) { | |
| const running = await listConversationMetas({ status: "running", limit: 1000 }); | |
| for (const m of running) { | |
| _runningCounts.set(m.agentSlug, (_runningCounts.get(m.agentSlug) ?? 0) + 1); | |
| } | |
| _runningCountsBootstrapped = true; | |
| } | |
| if (!_runningCountsBootstrapped) { | |
| _runningCountsBootstrapPromise ??= (async () => { | |
| const running = await listConversationMetas({ status: "running", limit: 1000 }); | |
| _runningCounts.clear(); | |
| for (const m of running) { | |
| _runningCounts.set(m.agentSlug, (_runningCounts.get(m.agentSlug) ?? 0) + 1); | |
| } | |
| _runningCountsBootstrapped = true; | |
| })(); | |
| await _runningCountsBootstrapPromise; | |
| } |
🤖 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 `@src/lib/agents/conversation-store.ts` around lines 1818 - 1824, The bootstrap
logic guarded by _runningCountsBootstrapped has a race condition where multiple
concurrent callers can all observe the flag as false and execute the bootstrap
concurrently, causing duplicate increments in _runningCounts. Fix this by
introducing a shared in-flight promise variable (e.g.,
_runningCountsBootstrapPromise) that is set before the bootstrap begins. Check
if this promise exists and await it if it does; otherwise, create the promise,
execute the bootstrap logic that fetches running conversations and populates
_runningCounts, and then mark _runningCountsBootstrapped as true. This ensures
only one bootstrap execution occurs while concurrent callers wait for the same
promise to resolve.
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.
Summary
getRunningConversationCountswas opening everymeta.jsonacross every cabinet on every 3-second SSE tick with no concurrency limit. Under multiple open rooms, the burst exhausted the OS fd limit (EMFILE / os error 24), which also starved turbopack's SST file reads.Mapbootstrapped once at startup;enqueueConversationNotificationdelta-updates it on every status transition — no disk reads on subsequent ticks.mapCappedto bound concurrent file opens inlistConversationMetasduring the bootstrap scan (4 cabinets × 20 entries in flight max).Test plan
Summary by CodeRabbit