fix(executors): rotate to the next account on network throws, not just 429 - #10402
Conversation
a676182 to
ee58be1
Compare
… account has a dedicated proxy OpencodeExecutor and MimocodeExecutor rotated to the next account only on HTTP 429. A network exception (timeout, connection refused/reset) on one account instead propagated out of execute() and failed the whole request, even when other accounts remained available. Both executors now rotate on a network exception only when the failed account has its own dedicated proxy (account.proxy !== null) — a dead proxy is genuinely account-scoped, so rotating away from it is safe. Accounts sharing the default egress (no proxy configured) trigger the same cooldown and are skipped for the rest of the request once the shared egress is known down, but a later account with its own dedicated proxy is still tried normally — a throw on a proxy-less account no longer strands a proxied account further in the rotation. This behavior is gated behind NETWORK_ROTATION_SHARED_EGRESS_GUARD (Feature Flag, default on); disabled, it reproduces the immediate-propagation behavior this fix started from. The shared rotation mechanics (pickAccount/markCooldown/markSuccess) are extracted into executors/accountRotation.ts, used by both executors — they had independently implemented the same round-robin+cooldown skeleton. This also fixes an identical, pre-existing bug in MimocodeExecutor that predates this PR: its catch block called markCooldown unconditionally on any throw, with no proxy check and no warn log (a silent exception swallow on a path that influences the result). The cooldown formula for both the proxy and shared-egress cases reuses the repo's already-established "transient, not clearly attributable" constants (errorConfig.ts TRANSIENT_COOLDOWN_MS/COOLDOWN_MS.transientMax, already used by accountFallback.ts for network-error classification) instead of introducing a separate value. MimocodeExecutor's network-error 502 body also now goes through buildErrorBody()/sanitizeErrorMessage() instead of embedding the raw caught error message directly (Hard Rule diegosouzapw#12), matching the sanitization already used on its diegosouzapw#2101 malformed-request path. Validated by TDD (Hard Rule diegosouzapw#18): tests/unit/account-rotation.test.ts covers the shared module directly; opencode-proxy-rotation-4954.test.ts and mimocode-executor.test.ts cover the proxy-configured rotation path, the mixed-fleet case, the shared-egress single-network-call case, and the NETWORK_ROTATION_SHARED_EGRESS_GUARD-disabled legacy path, for each executor. tsc, lint, and the provider golden-path gates (check:provider-consistency, check:provider-assets, provider-translate-path-golden.test.ts) are clean on all touched files.
ee58be1 to
0358c0e
Compare
maxmad64bis
changed the title
|
Thanks for this — nice catch, and a clean fix. I confirmed the underlying bug directly in the diff: the previous I ran the new/updated test files locally ( Two very minor, non-blocking notes, no action needed from you:
The Overall this looks merge-ready to me. Nice work, especially on the shared-egress guard nuance and the parity fix for MimocodeExecutor's error-body sanitization along the way. |
94cf4c4
into
diegosouzapw:release/v3.8.50
|
Validated in local merge-train |
…ocode-sunset-provider Resolves conflicts from the mimocode provider sunset removal overlapping with the account-rotation refactor (diegosouzapw#10402) and provider-count drift across docs/i18n mirrors, README, AGENTS.md, package.json and the generated PROVIDER_REFERENCE.md/diagram SVGs. Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
OpencodeExecutor treated a malformed upstream 400 (completion envelope with no error field, empty content, null finish_reason) as a successful response and propagated it fatally — killing client/subagent sessions on free-tier models. - Add isEmptyUpstreamRejection / extractChatcmplId predicates in accountRotation.ts (strict signature: status 400, no error field, no real content/tool_calls, null finish_reason; conservative on other content shapes). - Loop now retries/rotates on an empty 400 (bounded +1 only for a single account; multi-account rotation through the fleet is the retry). Body is read only for a 400 (never a 200/streaming) so the success path is never buffered. - Fast path (single direct account, no proxies) retries exactly once on an empty 400. - 400s carrying a real error field still propagate immediately, untouched. - Clean up the now-obsolete Mimocode mention in the accountRotation header (diegosouzapw#10130). Tests: accountRotation predicate suite + OpencodeExecutor wiring suite (rotation, bounded retry, fast path, anti-bufferisation, error-400 passthrough) against the existing proxy-rotation-4954 fetch-stub harness. Refs upstream precedent diegosouzapw#10402 (rotate beyond 429), diegosouzapw#10460 (classify 400 by signature before rotating).
2 participants
Summary
If a request to a free "OpenCode"/"MiMoCode" account hit a network error (timeout, connection
reset) instead of a rate limit, the whole request failed immediately — even when the user had
other accounts configured that could have handled it. Only HTTP 429 triggered account rotation;
a network error did not.
Now a network error rotates to the next account too, same as a 429 does. The one nuance: if the
failing account has no proxy of its own, the next few accounts likely share the same network path
and would fail the same way, so we don't hammer through all of them — we cool down and move
straight to an account that has its own proxy (if one exists) instead of retrying doomed accounts.
This is on by default and can be turned off (
NETWORK_ROTATION_SHARED_EGRESS_GUARDflag) to getthe old immediate-failure behavior back.
Also fixes the same bug in
MimoCodeExecutor, which had it too but silently — no log at all, sooperators couldn't see why a request failed.
Related Issues
Validation
check:provider-consistency,check:provider-assets,provider-translate-path-golden.test.ts, plus the executor/rotation/feature-flag test suitesbelow — all passing
npm run lintclean on touched filesrelease/v3.8.50tip, focused checks rerun afterTests Added Or Updated
tests/unit/account-rotation.test.ts(new) — shared rotation logic used by both executorstests/unit/opencode-proxy-rotation-4954.test.ts/tests/unit/mimocode-executor.test.ts—network-error rotation, mixed proxy/no-proxy accounts, and the guard-disabled fallback
tests/unit/feature-flags-settings.test.ts— the new flag's definitionReviewer Notes
open-sse/executors/accountRotation.ts.