fix: track spawned profile ID to correctly attribute rate limit errors#1994
fix: track spawned profile ID to correctly attribute rate limit errors#1994octo-patch wants to merge 1 commit intoAndyMik90:developfrom
Conversation
When a task process hits a rate limit, detectRateLimit() was called without a profileId and fell back to getActiveProfile(). If the user switched profiles after the task started, the rate limit was attributed to the wrong profile. Fix by returning profileId from setupProcessEnvironment(), recording it via state.assignProfileToTask() at spawn time, then passing it to detectRateLimit() in handleProcessFailure(). Fixes AndyMik90#1903
|
|
📝 WalkthroughWalkthroughThe PR fixes a bug where rate limit errors were incorrectly attributed to the inactive "Primary" profile instead of the active profile by enhancing profile tracking throughout the spawn and failure handling flow. Changes ensure spawned process tasks are assigned their correct profile and rate limit detection uses the proper profile context. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the AgentProcessManager to track and attribute Claude profiles to specific tasks by modifying setupProcessEnvironment to return profile metadata and recording these assignments in the agent's state. Feedback indicates that the rate limit detection logic needs to be integrated into the primary spawnWorkerProcess execution path, as it is currently only present in a deprecated method. Additionally, the new task-to-profile assignments must be cleared upon process completion to prevent a memory leak.
| const profileAssignment = this.state.getTaskProfileAssignment(taskId); | ||
| const rateLimitDetection = detectRateLimit(allOutput, profileAssignment?.profileId); |
There was a problem hiding this comment.
The handleProcessFailure method is currently only invoked from the deprecated spawnProcess method. However, spawnWorkerProcess (line 838) is the primary execution path for autonomous AI tasks and it does not appear to utilize this rate limit detection and attribution logic. To fully address the issue described in the PR, the worker-based execution path should also be updated to track and attribute rate limits correctly.
|
|
||
| const env = this.setupProcessEnvironment(extraEnv); | ||
| const { env, profileId: spawnedProfileId, profileName: spawnedProfileName } = this.setupProcessEnvironment(extraEnv); | ||
| this.state.assignProfileToTask(taskId, spawnedProfileId, spawnedProfileName, 'proactive'); |
There was a problem hiding this comment.
While calling assignProfileToTask here ensures correct attribution during failure, the assignment is never removed from the taskProfileAssignments map in AgentState. This will result in a memory leak as the map grows with every task execution. Please ensure that this.state.clearTaskProfileAssignment(taskId) is called in the process exit and error handlers (e.g., near lines 777 and 812).
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
|
|
||
| const env = this.setupProcessEnvironment(extraEnv); | ||
| const { env, profileId: spawnedProfileId, profileName: spawnedProfileName } = this.setupProcessEnvironment(extraEnv); | ||
| this.state.assignProfileToTask(taskId, spawnedProfileId, spawnedProfileName, 'proactive'); |
There was a problem hiding this comment.
Bug: The call to assignProfileToTask() was added to the deprecated spawnProcess method instead of the spawnWorkerProcess method used by production code, so profile assignments are not tracked.
Severity: MEDIUM
Suggested Fix
Move the assignProfileToTask() call from the deprecated spawnProcess method to the spawnWorkerProcess method. This will ensure that all production tasks have their associated profile ID correctly tracked for error handling.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: apps/desktop/src/main/agent/agent-process.ts#L554
Potential issue: The fix to track the profile ID for a spawned task was added to the
`spawnProcess` method. However, this method is deprecated and no longer used in
production. All production tasks are created via `spawnWorkerProcess`, which was not
modified to call `assignProfileToTask()`. As a result, when a task fails (e.g., due to a
rate limit), `handleProcessFailure` cannot retrieve the correct profile ID. The system
then falls back to the currently active profile, causing rate limit errors to be
misattributed if the user has switched profiles since the task began.
Did we get this right? 👍 / 👎 to inform future reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/main/agent/agent-process.ts (1)
838-863:⚠️ Potential issue | 🟠 MajorAdd profile tracking to
spawnWorkerProcessfor rate limit attribution consistency.The fix adds
setupProcessEnvironment()andassignProfileToTask()tospawnProcess(line 553–554), butspawnWorkerProcess(lines 838–880) does not call these methods. Worker processes make SDK calls (via oauth-fetch, which operates in worker threads) and can trigger rate limits, but their exit handler (lines 889–911) lacks rate limit detection viahandleProcessFailure().Without profile tracking, if workers hit rate limits, those errors won't be attributed to the correct profile. Apply the same profile setup pattern to
spawnWorkerProcessasspawnProcessfor consistency and complete rate limit attribution.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/agent/agent-process.ts` around lines 838 - 863, spawnWorkerProcess currently adds the process to state but does not call setupProcessEnvironment() or assignProfileToTask(), so worker-thread SDK calls won't be attributed to profiles; update spawnWorkerProcess to mirror spawnProcess by invoking setupProcessEnvironment(taskId, executorConfig, extraEnv, processType, projectId) before starting the worker and then call assignProfileToTask(taskId, profile) (use the same profile object returned/used by setupProcessEnvironment), and ensure the worker's exit/error handling routes failures through handleProcessFailure(taskId, spawnId, error) so rate-limit errors from worker threads are detected and attributed correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/desktop/src/main/agent/agent-process.ts`:
- Around line 838-863: spawnWorkerProcess currently adds the process to state
but does not call setupProcessEnvironment() or assignProfileToTask(), so
worker-thread SDK calls won't be attributed to profiles; update
spawnWorkerProcess to mirror spawnProcess by invoking
setupProcessEnvironment(taskId, executorConfig, extraEnv, processType,
projectId) before starting the worker and then call assignProfileToTask(taskId,
profile) (use the same profile object returned/used by setupProcessEnvironment),
and ensure the worker's exit/error handling routes failures through
handleProcessFailure(taskId, spawnId, error) so rate-limit errors from worker
threads are detected and attributed correctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7fb2bb16-c3fa-466c-b8b0-2cc2b68c54be
📒 Files selected for processing (1)
apps/desktop/src/main/agent/agent-process.ts
2 participants
Fixes #1903
Problem
When a task process hits a rate limit,
detectRateLimit()was called inhandleProcessFailure()without passing theprofileIdof the profile that spawned the process. It fell back toprofileManager.getActiveProfile().id, which may have changed since the process was spawned.Concrete scenario: A user creates a Z.AI profile and switches to it. Tasks still running under "Primary"'s credentials hit Primary's rate limit. Because
getActiveProfile()now returns Z.AI, the error gets attributed to Z.AI instead of Primary — showing the user an incorrect "Profile: Primary is rate limited" notification even while they believe they are using Z.AI only.The
AgentState.assignProfileToTask()mechanism already existed to track which profile was assigned to each task, but it was never called fromspawnProcess().Solution
setupProcessEnvironment()to return{ env, profileId, profileName }so the caller knows which profile was selected bygetBestAvailableProfileEnv().spawnProcess(), record the profile assignment immediately after environment setup viathis.state.assignProfileToTask().handleProcessFailure(), look up the stored profile assignment and pass itsprofileIdtodetectRateLimit(), so rate limit events are correctly attributed to the profile that spawned the process.Testing
The existing tests in
agent-process.test.tsalready mockgetBestAvailableProfileEnv()to return{ env: {}, profileId: 'default', profileName: 'Default', wasSwapped: false }, which is compatible with the updated return type — no test changes needed.Summary by CodeRabbit