fix(ali): stop injecting top_p into requests that omit it - #6674
Conversation
DashScope rejects top_p at the 0 and 1 boundaries, so requestOpenAI2Ali clamped the value into the open interval. It read the pointer through lo.FromPtrOr(request.TopP, 0), so a request that omitted top_p was indistinguishable from one that sent 0 and got 0.001 injected. That had two effects. Models on the platform that validate two decimals rejected the request with "top_p参数非法:限制小数点[2]位". Models that do not validate it silently ran at near-greedy decoding instead of their own default. Only clamp when the caller actually sent top_p, and clamp to two decimals.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughThe Ali request conversion now preserves omitted ChangesAli top_p handling
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
* fix(relay): set Request.GetBody so the HTTP/2 transport can transparently retry after an upstream stream reset (QuantumNous#6249) * fix(relay): set Request.GetBody so the HTTP/2 transport can transparently retry after an upstream stream reset The outbound request body is a type-erased io.Reader over BodyStorage, so net/http cannot derive Request.GetBody (it only does so for *bytes.Reader, *bytes.Buffer and *strings.Reader). With GetBody nil, the HTTP/2 transport cannot transparently retry a request once the body has been written and the upstream resets the stream with a retryable error (REFUSED_STREAM, or a connection-level GOAWAY); the relay request then fails with: http2: Transport: cannot retry err [...] after Request.Body was written; define Request.GetBody to avoid this error This affects every relay path that goes through DoApiRequest (chat, claude, gemini, responses, embedding, image, rerank). BodyStorage (memory and disk) already implements io.Seeker, so replay support only needed wiring: - NewOutboundJSONBody additionally returns a getBody that rewinds the storage and hands out a fresh non-closing reader. The transport only calls GetBody after the previous attempt's body has been abandoned, so the rewind cannot race an in-flight read. - RelayInfo carries it in the new UpstreamRequestGetBody field, set alongside UpstreamRequestBodySize by the handlers that build storage-backed bodies. - applyUpstreamGetBody (symmetric with applyUpstreamContentLength) wires it into DoApiRequest/DoFormRequest/DoTaskApiRequest, only when req.GetBody is still nil. Also remove the hand-rolled GetBody override in DoTaskApiRequest: it returned the same already-consumed reader, so any transport-level replay would have silently sent an empty body, and it clobbered the correct snapshot-based GetBody that net/http derives from the *bytes.Reader bodies the task adaptors pass in. For non-replayable bodies GetBody now stays nil, so a retry fails loudly instead of corrupting the request. Covered by unit tests plus an end-to-end raw-frame HTTP/2 test that resets the first stream with REFUSED_STREAM after the body is written and asserts the transport transparently retries with the complete body. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(relay): hand out independent readers from GetBody (address review) Per the http.Request.GetBody contract ("returns a new copy of Body"), each call must yield a reader with its own cursor. The previous implementation rewound and reused the shared BodyStorage, so two consecutive GetBody readers would interfere with each other, and a replay could disturb the primary body's offset under extreme transport timing (e.g. attempt N's body write not yet fully abandoned when the transport builds attempt N+1). Instead of snapshotting the payload (an extra copy), add BodyStorage.NewReader, which returns an independent zero-copy reader: - memory mode: a fresh bytes.Reader over the same immutable backing array; - disk mode: a separate file descriptor over the cache file, so the transport closing a replayed body only closes that descriptor. NewOutboundJSONBody's getBody now simply hands out storage.NewReader, and once the handler releases the storage, GetBody fails with ErrStorageClosed instead of replaying stale data. Tests: interleaved reads across two replay readers and the primary body each observe exactly their own byte stream, for both the memory and the disk-backed storage; the existing GetBody and HTTP/2 retry suites still pass (h2 e2e tests flake-free with -count=20). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(relay): bind replayable metadata on pass-through requests * fix(relay): reset upstream body metadata between channels * test(relay): cover replay across retries and channel attempts * fix(relay): stop following upstream redirects --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> * refactor(relay): move replay metadata onto request bodies * Merge commit from fork * feat(channels): refine fetched model categorization (QuantumNous#6632) * feat(channels): refine fetched model categorization * fix: channel category * fix: hy3 category * fix: test Claude/Gemini endpoints with native request format (QuantumNous#6698) * feat(rate-limit): add user critical rate limit middleware for access token and aff transfer routes * fix: 修复兑换码额度精度损失 (QuantumNous#6685) * fix: 修复兑换码额度精度损失(QuantumNous#6680) * fix(redemption): guard update data integrity * CI: enhance release synchronization workflow with optional file syncing * fix(ali): stop injecting top_p into requests that omit it (QuantumNous#6674) * fix(channels): classify Qwen TTS models correctly (QuantumNous#6711) * feat(channels): add auto-disable-only channel test mode (QuantumNous#6728) * perf(web): debounce server and large-list searches (QuantumNous#6727) * fix: record reasoning effort consistently in usage logs (QuantumNous#6641) * feat(relay): expose user and group context to parameter overrides (QuantumNous#6534) * fix(ollama): preserve reasoning and tool-call context (QuantumNous#6605) * fix: backend length validation (QuantumNous#5548) * feat(billing): highlight matched conditional multipliers in logs (QuantumNous#6561) * feat(billing): highlight matched conditional multipliers in usage logs * fix(billing): make request rule tracing stable and type-safe * fix(web): require confirmation before rotating access token (QuantumNous#6749) --------- Co-authored-by: Lucas <hepo.lucas@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: CaIon <i@caion.me> Co-authored-by: RedwindA <128586631+RedwindA@users.noreply.github.com> Co-authored-by: Seefs <40468931+seefs001@users.noreply.github.com> Co-authored-by: lihu-001 <lihu9048@gmail.com> Co-authored-by: ENCHIGO <38551565+ENCHIGO@users.noreply.github.com>
Brings the fork up to upstream v1.0.0-rc.25 (47 commits, 233 files). Patches: - drop 6674-ali-top-p-passthrough (merged upstream as 2399de9) - drop 6573-custom-oauth-binding-status (superseded by upstream QuantumNous#6818, which aligned provider_id/provider_user_id in the frontend) - the remaining 12 still apply cleanly, in order, on rc.25 Conflict resolutions: - backend: took upstream's logic and re-applied our English log/error strings (atomic top-up settlement, Midjourney refund via service.RefundMidjourneyQuota, rune-based console length validation, common.QuotaFromFloat/QuotaFromDecimalStrict) - controller/channel-test.go: took upstream's worker-pool rewrite and re-applied ElevenLabs support plus our English strings - relaykit oai_chat request conversion: kept our web_search server-tool mapping alongside upstream's parameterless-tool handling; unioned both sides of the new test file - locales: 3-way union, 111 new upstream keys translated to pt-BR Frontend now runs on upstream's Vitest setup (jsdom): - converted three node:test files to Vitest - dropped the per-file happy-dom bootstrap that shadowed jsdom - test-setup installs an in-memory Storage because Bun's built-in localStorage throws and shadows jsdom's Also documents why a bare `patch --dry-run` reports success on macOS for an already-merged patch, which is what hid QuantumNous#6674. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CyvTzK8hTXFkGc7sWGg9ma
Important
📝 变更描述 / Description
requestOpenAI2Ali需要把top_p收进开区间,因为 DashScope 不接受 0 和 1 这两个边界值。但它是用lo.FromPtrOr(request.TopP, 0)读取指针的,这样**"没传 top_p"和"传了 0"就无法区分**——两者都会落进topP <= 0分支,被注入0.001。这带来两个后果:
0.001/0.999都是三位小数,平台上部分模型只接受两位,会以top_p参数非法:限制小数点[2]位拒绝整个请求。实测ZHIPU/GLM-5.2、xiaomi/mimo-v2.5-pro在客户端不传top_p时 100% 失败。top_p被设成0.001,等价于近乎贪婪解码,取代了模型自己的默认值。调用方无从察觉。改法是把边界收敛限定在"调用方确实传了
top_p"的情况,并把 clamp 值改成两位小数:request.TopP == nil时原样返回,交由上游使用模型默认值 → 修复第 2 点0.01/0.99→ 修复第 1 点ThinkingBudget的处理逻辑未改动。🚀 变更类型 / Type of change
🔗 关联任务 / Related Issue
✅ 提交前检查项 / Checklist
type=17)的 chat 请求转换。显式传入的top_p行为只在边界值上由三位小数变为两位小数;未传top_p的请求改为不再携带该字段,由上游使用模型默认值。relay/channel/ali/text.go与新增对应单元测试,无其他无关改动。📸 运行证明 / Proof of Work
新增
relay/channel/ali/text_test.go,覆盖 6 种输入:top_pnil)nil0.80.81.00.991.50.990.00.01-0.30.01另在生产环境验证过等效行为:通过渠道
param_override条件删除被注入的top_p后,ZHIPU/GLM-5.2与xiaomi/mimo-v2.5-pro立即恢复正常,同渠道其余 11 个模型不受影响。Summary by CodeRabbit
Bug Fixes
top_psetting.0.01and0.99.Tests