feat: per-OAuth-account proxy assignment for IP diversity#3140
feat: per-OAuth-account proxy assignment for IP diversity#3140pizzav-xyz wants to merge 5 commits into
Conversation
- Add OAuthProxy config struct (proxies + accounts-per-proxy) - Validate count match at startup: len(auths) == accounts_per_proxy * len(proxies) - Assign proxies to auths sorted alphabetically by filename - Log assignment table at INFO level This enables multi-account users to distribute requests across different proxies for IP diversity without manual configuration.
validation was running before Load() populated the auth list, causing false mismatch errors
|
This pull request targeted The base branch has been automatically changed to |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dca8dd210d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| log.Warnf("failed to load auth store: %v", errLoad) | ||
| } | ||
| // Apply proxy assignment after loading auths. | ||
| s.applyOAuthProxyAssignment() |
There was a problem hiding this comment.
Run() applies proxy assignments before checking whether oauth-proxy is valid, so a mismatched config still rewrites and persists auth entries via coreManager.Update(...) and only then returns an error. In the mismatch scenario (e.g., wrong accounts-per-proxy), startup fails but leaves partially/incorrectly reassigned proxy state on disk, which is a harmful side effect for what should be a pure validation failure.
Useful? React with 👍 / 👎.
| if _, err := s.coreManager.Update(context.Background(), auth); err != nil { | ||
| log.Warnf("failed to update auth %s with proxy assignment: %v", auth.ID, err) | ||
| } | ||
| log.Infof("oauth-proxy assignment: %s -> %s", auth.ID, proxies[proxyIndex]) |
There was a problem hiding this comment.
The info log prints the full proxy URL for each auth assignment. If users configure authenticated proxies (for example socks5://user:pass@host:port), this emits secrets into normal logs and log aggregation systems. Since proxy credentials are sensitive, this should be redacted or omitted.
Useful? React with 👍 / 👎.
![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 implements a proxy assignment system for OAuth accounts to enhance IP diversity, adding configuration structures and logic to distribute accounts across proxies in sequential blocks. The review feedback identifies several improvement opportunities, including optimizing the update loop for performance, ensuring proper context propagation, correcting misleading documentation regarding the assignment method, and reordering logic to validate configuration before execution to follow the fail-fast principle.
| proxyIndex = proxyIndex % len(proxies) | ||
| } | ||
| auth.ProxyURL = proxies[proxyIndex] | ||
| if _, err := s.coreManager.Update(context.Background(), auth); err != nil { |
There was a problem hiding this comment.
This Update call has two potential issues:
- Performance: Calling
Updateinside a loop can be inefficient if there are many auth files, as it likely performs I/O for each. IfcoreManagercan support it, a batch update would be more performant. - Context: It uses
context.Background(). TheapplyOAuthProxyAssignmentfunction should accept acontext.Contextfrom its caller (Run) and pass it tos.coreManager.Updatefor proper context propagation and cancellation handling.
| Payload PayloadConfig `yaml:"payload" json:"payload"` | ||
|
|
||
| // OAuthProxy configures per-OAuth-account proxy assignment for IP diversity. | ||
| // Auth files are sorted alphabetically, then assigned to proxies in round-robin fashion. |
There was a problem hiding this comment.
The comment "assigned to proxies in round-robin fashion" could be misleading. The implementation in applyOAuthProxyAssignment uses proxyIndex := i / accountsPerProxy, which is a sequential block assignment (e.g., first N accounts to proxy 1, next N to proxy 2, etc.), not a typical item-by-item round-robin (i % num_proxies). To avoid confusion, please clarify the assignment method in the comment. For example: "...then assigned to proxies in sequential blocks, with each proxy receiving accounts_per_proxy accounts."
| // OAuthProxy defines per-OAuth-account proxy assignment for IP diversity. | ||
| type OAuthProxy struct { | ||
| // Proxies is the list of proxy URLs to assign to OAuth accounts. | ||
| // Each proxy URL is assigned in round-robin fashion based on sorted auth file order. |
There was a problem hiding this comment.
As noted in another comment, the term "round-robin fashion" can be misleading given the implementation. The assignment is sequential and block-based. Please update the comment to accurately describe the assignment logic to improve clarity for future maintainers.
| if proxyIndex >= len(proxies) { | ||
| proxyIndex = proxyIndex % len(proxies) | ||
| } |
There was a problem hiding this comment.
This if block is redundant. The validateOAuthProxyConfig function ensures that len(auths) == len(proxies) * accountsPerProxy. Given this validation, the proxyIndex calculated as i / accountsPerProxy will always be within the bounds of the proxies slice, so this check is unnecessary and can be removed.
| // Apply proxy assignment after loading auths. | ||
| s.applyOAuthProxyAssignment() | ||
| } | ||
|
|
||
| // Validate OAuthProxy config after loading auths. | ||
| if err := s.validateOAuthProxyConfig(); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
For better adherence to the fail-fast principle, validation should occur before any action is taken. Please move the validateOAuthProxyConfig() call and its error handling to be before the applyOAuthProxyAssignment() call. This ensures that if the configuration is invalid, the service fails fast without attempting to apply proxy assignments.
- validate before mutation (fail-fast) - redact proxy URLs in logs - pass context to Update - remove redundant bounds check
|
Disclaimer: I am an AI assistant, not a human. Please verify all code changes before merging. Fix UpdateAll review comments addressed in commit
Fork PR with fixes: pizzav-xyz#1 |
|
oops sorry my clanker has gone rouge but i feel like this is still a good pr |
luispater
left a comment
luispater
left a comment
There was a problem hiding this comment.
Summary
Adds an oauth-proxy config block to assign per-auth proxies (N accounts per proxy) for IP diversity.
Blocking
- The current implementation persists assignments into the auth store via
coreManager.Update(...)(writesAuth.ProxyURLback). This makesoauth-proxyeffectively a one-way mutation of auth files: removing/changingoauth-proxylater will not automatically revert previously writtenproxy_urlvalues. Please clarify intended semantics. If this is meant to be runtime-only policy, consider implementing the mapping via a customRoundTripperProviderinstead of persisting auth JSON, or provide an explicit rollback/clear behavior. - Misconfiguration is silently ignored when only one of
proxies/accounts-per-proxyis set (oraccounts-per-proxy <= 0). Prefer failing fast with a clear config error to avoid surprising no-op behavior.
Non-blocking
- Comments mention sorting by auth file name, but the code sorts by
auth.ID. Consider sorting byFileNameor updating the wording. - Add an
oauth-proxysnippet toconfig.example.yamlto make the feature discoverable. - Align error text with the YAML key
accounts-per-proxy.
Test plan
Not run locally (review only).
Blocking: - oauth-proxy now runtime-only via proxyMapping (no Update/persistence) - fail-fast on partial config (one of proxies/accounts-per-proxy set) Non-blocking: - error text aligned with accounts-per-proxy - sort by FileName not auth.ID - oauth-proxy snippet added to config.example.yaml
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26d136f566
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return err | ||
| } | ||
| // Apply proxy assignment only after validation passes. | ||
| s.applyOAuthProxyAssignment(ctx) |
There was a problem hiding this comment.
oauth-proxy assignment is only executed once at startup, so subsequent config or auth-file changes handled by the watcher do not refresh (or clear) proxyMapping. In practice, if an operator edits oauth-proxy (including disabling it) or adds/removes auth entries while the process is running, RoundTripperFor will keep using stale overrides for old IDs and no overrides for new IDs, which breaks the intended per-account proxy routing until a full restart.
Useful? React with 👍 / 👎.
recompute proxyMapping when: - config is reloaded (reloadCallback) - auth files change (handleAuthUpdate on add/modify/delete) fixes Codex P1: stale proxy mapping after runtime config changes
2 participants
Summary
Changes
Files changed (3)
Test