Skip to content

fix(qoder): honor proxy in direct chat, sync watchdog readiness, and keep canonical model IDs - #404

Open
longlixiao-ong wants to merge 4 commits into
caigee-cmd:mainfrom
longlixiao-ong:fix/qoder-routing-proxy-and-liveness
Open

longlixiao-ong wants to merge 4 commits into
caigee-cmd:mainfrom
longlixiao-ong:fix/qoder-routing-proxy-and-liveness

Conversation

@longlixiao-ong

Copy link
Copy Markdown
Contributor

Summary

This PR addresses three critical issues in Qoder request execution, proxy routing, and worker process lifecycle:

  1. Proxy Support for In-Process Qoder Chat (Direct):

    • qoderClient.SetDirect previously created a Direct client without wiring SetProxy.
    • In environments requiring a proxy (e.g. forward proxy configured per-account or globally via proxy_url secret), in-process chat directly to api1.qoder.sh / gateway.qoder.com.cn bypassed proxy settings and timed out.
    • Wired qoderDirect.SetProxy and proxyutil.TransportCache in Direct to honor account/system proxy configurations.
  2. Watchdog Liveness & Cold-Boot Readiness Synchronization:

    • At startup, manager.RefreshAll ran immediately concurrent with worker node process spawn, before ports were bound. The initial connection refusal marked items as Ready = false in the routing pool.
    • The 30s watchdog probeWorkerLiveness checked liveness (probeHealthOnly) but never merged Ready = true back into m.pool or fetched catalog models when the warming worker finally became ready, leaving accounts permanently out of request routing.
    • Added a 3-second startup grace period in app.go and updated probeHealthOnly to merge Ready = true and populate models into m.pool once the worker responds ready/hot.
  3. Prevent Unwanted Native Model Key Rewriting for Qoder:

    • sanitizeForItem unconditionally rewrote req.Model with NativeModelID(item, req.Model), which mapped qwen3.8-max to qmodel_38max, qwen3.8-flash to qfmodel, etc.
    • Qoder upstream chat gateway (api1.qoder.sh and gateway.qoder.com.cn) expects canonical public model names. Sending internal CLI enum keys caused the gateway to hang for 180 seconds until resetting with HTTP/2 INTERNAL_ERROR.
    • Bypassed NativeModelID replacement for Qoder in sanitizeForItem so public model IDs are forwarded directly.

Validation

  • go test ./... passed.
  • go test ./internal/app -run 'TestImportConstraints|TestDutyBoundaries' -count=1 passed.
  • python3 scripts/release-notes.py validate passed.
  • Live verified against both streaming and non-streaming Qoder models with low latency (<2s).

@caigee-cmd caigee-cmd left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three unrelated fixes in one PR. The proxy wiring is the real gap; the other two should not land as written.

1. Direct proxy — yes, land this.
qoder.Direct currently Dos on a bare client, so account / system proxy_url never reaches in-process chat. Wiring SetProxy + proxyutil.TransportCache the same way WorkBuddy does is the correct fix for timeouts on api1.qoder.sh / gateway.qoder.com.cn behind a forward proxy.

2. Watchdog Ready merge — right diagnosis, rough implementation.
probeHealthOnly on main is explicitly “liveness only; readiness stays refreshOne's job”. A boot RefreshAll racing the worker bind can stamp Ready=false, and the 30s watchdog then never writes Ready=true back. Merging health on a successful probe is the right recovery.

Please drop the 3-second time.Sleep before RefreshAll. It delays in-process quota refresh for every non-Qoder account, and a worker that takes >3s to listen still fails the first probe. If the watchdog writes Ready, the sleep is unnecessary. Also:

  • go m.fetchAccountModels(context.Background(), item) has no timeout and no single-flight.
  • The long comment above probeHealthOnly still says this path must not touch pool Ready — update it if the contract changes.
  • No test for the Ready merge.

3. Skip NativeModelID for Qoder — do not land until proven.
sanitizeForItem rewriting qwen3.8-max → qmodel_38max is not what the Direct path sends. Direct.prepare still runs resolveCatalogModel, which sets req.Model to the catalog mapped_key (existing tests assert qmodel_38max). Skipping NativeModelID in the executor only changes the body when the catalog is empty/unresolved. If enum keys really hang the gateway for 180s, the rewrite to drop is in resolveCatalogModel, not sanitizeForItem.

The hang you saw is more likely the missing proxy (fix 1). Please capture the outbound model / X-Model-Key for a catalog-hit request and a catalog-miss request before changing ID policy.

Ask: split this PR. Land the Direct proxy (and, if you want, the Ready merge without the sleep). Leave the model-ID bypass out until the capture shows the gateway actually received qmodel_38max.

@caigee-cmd caigee-cmd left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Architecture follow-up, against AGENTS.md / docs/ARCHITECTURE_SUMMARY.md. This is why the three hunks cannot land together.

Direct proxy — fits the split.
app wires the client; providers/qoder.Direct owns the upstream HTTP; executor is not involved. Same shape as WorkBuddy's TransportCache. Qoder chat stays the in-process client. Land this.

Watchdog Ready — right package, wrong contract.
Pool Ready belongs in runtime, not app or executor. The 3s time.Sleep in app.New does not: process assembly must not paper over worker bind races. Warmup/liveness lives in runtime.Start / the maintenance loop. probeHealthOnly is documented as liveness-only; readiness stays refreshOne. If that contract changes, change it in runtime (comment, timeout, single-flight) and test it. Do not sleep in app.New.

Skip NativeModelID for Qoder — executor must not carry provider protocol.
sanitizeForItem is executor request-prep. Native catalog spelling is provider mapping (Direct.prepare → resolveCatalogModel already rewrites to mapped_key). if itemProvider(item) != "qoder" in executor is a provider special case in the wrong package: “provider 负责上游事实映射;executor 负责是否切号”. Do not add Qoder-only ID policy to executor. If the gateway truly rejects enum keys, fix resolveCatalogModel in providers/qoder.

Keep this PR to Direct proxy (+ optional Ready merge entirely inside runtime, no app sleep). Drop the executor model-ID bypass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants