[MAIN-Feat] Add proxy manager to job consumer#101
Conversation
update proxy injection to allow injecting different strategy and proxyId from the default passed via job params Add proxy deletion from axios instance Add spawning new axios instances with other proxy strategies
|
Warning Review limit reached
More reviews will be available in 43 minutes and 11 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughProxy injection is extracted into a new ProxyManager class with a constructor interface and methods for inject/re-inject/uninject. JobConsumer now imports ProxyManager, stores an optional proxyManager field, and uses it in preRun to manage per-job proxy interceptors. ChangesProxyManager Class Extraction and Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/jobConsumer/jobConsumer.ts (1)
96-115: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winClarify or remove the legacy
injectProxiesmethod.With
ProxyManagernow handling proxy injection inpreRun(line 249), this method is redundant and creates confusion about the preferred approach. If it's intentionally kept for backward compatibility or direct usage by subclasses, add a comment explaining its purpose; otherwise, consider deprecating or removing it.🤖 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/jobConsumer/jobConsumer.ts` around lines 96 - 115, The injectProxies method is now redundant because ProxyManager performs proxy injection in preRun; either remove injectProxies or explicitly mark it as deprecated/backward-compatible: add a clear comment above async injectProxies(proxyConfig?: jobProxyConfig) stating it is retained only for backward compatibility and will delegate to ProxyManager (or call injectProxy for compatibility) so subclasses know the preferred path, or remove the method entirely and update references to use ProxyManager.preRun; mention the related symbols injectProxies, ProxyManager, preRun, jobProxyConfig, and injectProxy so reviewers can locate and apply the change.
🤖 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/jobConsumer/jobConsumer.ts`:
- Around line 242-249: Validate that job.id is defined before constructing
ProxyManager (avoid using the non-null assertion job.id!); if undefined, log an
error via this.logEvent and skip proxy setup. Wrap the call to
this.proxyManager.injectProxies() in a try/catch, log success details (e.g.,
injected proxy IP/port or returned result) using this.logEvent when
injectProxies() resolves, and on failure catch the exception, log the error and
continue job setup instead of letting it throw. Ensure references to
ProxyManager, injectProxies(), this.proxyManager, injectProxy and getJobProxies
are preserved so downstream code receives a valid jobId or is skipped safely.
In `@src/utils/proxyUtils.ts`:
- Around line 178-192: injectProxiesIntoANewClient creates Axios instances via
defaultAxiosInstance.create() but never tracks them, which can leak resources or
make lifecycle management unclear; update the class to record each new instance
(e.g., push newInstance into a this.createdAxiosInstances array) after calling
injectProxy, and add a companion cleanup method (e.g.,
cleanupAxiosInstance(instance?: AxiosInstance) or cleanupAllAxiosInstances())
that will remove listeners/agents and/or delete entries from the array;
alternatively, document in the method comment that callers must manage instance
lifetimes—refer to injectProxiesIntoANewClient, defaultAxiosInstance, and
injectProxy when implementing the tracking and cleanup.
- Around line 194-198: uninjectProxies currently only removes proxy settings and
fails to eject the request interceptor registered by injectProxy; update
ProxyManager to store the interceptor id returned by injectProxy
(proxyUsageInterceptor) when injecting (in injectProxy, reInjectProxies, and
injectProxiesIntoANewClient) and then call eject with that stored id inside
uninjectProxies before deleting defaults.env?.fetch and defaults.proxy, ensuring
the stored interceptor id is cleared after ejection; reference the ProxyManager
class, methods injectProxy, reInjectProxies, injectProxiesIntoANewClient, and
uninjectProxies to locate where to save and later eject the interceptor.
---
Outside diff comments:
In `@src/jobConsumer/jobConsumer.ts`:
- Around line 96-115: The injectProxies method is now redundant because
ProxyManager performs proxy injection in preRun; either remove injectProxies or
explicitly mark it as deprecated/backward-compatible: add a clear comment above
async injectProxies(proxyConfig?: jobProxyConfig) stating it is retained only
for backward compatibility and will delegate to ProxyManager (or call
injectProxy for compatibility) so subclasses know the preferred path, or remove
the method entirely and update references to use ProxyManager.preRun; mention
the related symbols injectProxies, ProxyManager, preRun, jobProxyConfig, and
injectProxy so reviewers can locate and apply the change.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9e2d17b3-d586-4f57-b650-69fd30064d20
📒 Files selected for processing (3)
src/jobConsumer/jobConsumer.tssrc/types/proxies.tssrc/utils/proxyUtils.ts
…in that case Adding axios interceptor clearing when reinjecting proxies or clearing an existing proxy manager in the task instance.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/jobConsumer/jobConsumer.ts`:
- Around line 226-229: Fix the incomplete and grammatically incorrect inline
comment above the this.proxyManager check: replace "the previous one will have
it's axios interceptors" with a complete sentence and correct possessive to
"its" (e.g., "If there is an existing proxyManager in this instance, the
previous manager will still have its axios interceptors attached, so we clear
them before replacing it."). Update the comment near the this.proxyManager check
that calls this.proxyManager.clearInterceptors() to this clearer, corrected
wording.
In `@src/types/models/job.ts`:
- Line 4: Remove the unused import by deleting the "ProxyManager" import
statement or, if it was intended, incorporate ProxyManager into the types (e.g.,
add a field using ProxyManager to JobOptions or another relevant type) so the
symbol is referenced; update any affected exports (JobOptions, Job, etc.)
accordingly and run the linter to ensure no remaining unused-import warnings.
In `@src/utils/proxyUtils.ts`:
- Around line 209-223: In uninjectProxies, the current falsy check (if
(!interceptorId)) skips interceptorId === 0 and leaves the interceptor
registered and the instanceMap stale; change the check to explicitly test for
undefined (e.g., interceptorId === undefined) or use has/get semantics so
interceptorId 0 is considered valid, then call
targetInstance.interceptors.request.eject(interceptorId) and afterwards remove
the entry from this.instanceMap (this.instanceMap.delete(targetInstance)) to
mirror clearInterceptors and avoid stale IDs; keep the existing deletion of
targetInstance.defaults.env?.fetch and targetInstance.defaults.proxy logic and
return targetInstance.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 888024f5-60b2-42fc-9276-50ee9176d900
📒 Files selected for processing (4)
src/jobConsumer/jobConsumer.tssrc/types/models/job.tssrc/types/proxies.tssrc/utils/proxyUtils.ts
fixing stale interceptor instances when un-injecting proxies
1 participant
features:
Notes
None
Motivation and Context
The ProxyManager was added to better handle proxies when writing jobs. the proxy manager will, in the future, handle getting logs, emitting events related to proxies/networking, and proxy metrics.
The proxy manager also allows creating new clients with new proxy strategies outside the default
jobParamssupplied one.How Has This Been Tested?
Screenshots (if applicable)
Types of changes
Related Issues
Additional Context
Summary by CodeRabbit