feat(admin): make JSON request body limits runtime-tunable - #150
Conversation
6c2b319 to
00ecec6
Compare
|
@astaxie 辛苦及时考虑一下,现在 computer use 基本都没法用了 |
astaxie
left a comment
There was a problem hiding this comment.
发现 1 个需要修复的 P2 问题:新增的请求体大小设置未补齐英文和日文翻译,非中文界面会回退显示中文。除此之外,后端实现与 PR 描述一致,相关 CI 和新增的后端测试均通过。请补齐翻译后再合并。
00ecec6 to
60f4c78
Compare
|
@astaxie 看到这块已经由 #144 合入并 close 了 #125,所以我把这个 PR 整个 rebase 到当前 main 之上、重新定位为 #144 之上的运行时可调增强,不再重复 #144 的能力,而是补 #144 缺的"免重启动态调整"。 调整思路:
校验:gofmt/vet/build 干净、前端 typecheck+build 干净、 如果方向 OK,主要想确认一点:env 在我这里做的是"默认值"而非"硬上限"(管理员可调到高于 env、最高 512 MiB)。若你更希望 env 当硬上限(管理员只能在 env 以下调),我再改。其余细节欢迎指出。 |
There was a problem hiding this comment.
Re-reviewed the complete current diff at 60f4c78. The English/Japanese settings translations are now present, so the prior translation thread is resolved. This head still conflicts with current main, and the PR title should use an imperative summary, for example: feat(admin): make JSON request body limits runtime-tunable. Resolve the conflict, update the title, and rerun required CI before approval.
|
解决一下冲突,基本上就可以合并了 |
60f4c78 to
814d71c
Compare
astaxie
left a comment
There was a problem hiding this comment.
Reviewed the complete current diff at 814d71c after the new code-only update. This head is not merge-ready:
- [P1] The advertised 10-second cache never becomes fresh. readEffectiveBodyLimits returns a bodyLimitSnapshot without setting refreshedAt. Consequently currentBodyLimits always evaluates time.Since(snap.refreshedAt) < bodyLimitTTL as false and calls ListResources("settings") on every request. Set refreshedAt when a successful snapshot is built and add a regression that counts settings reads across repeated calls and after TTL expiry.
- [P1] Required CI is red. Frontend, Repository gates, and aggregate CI fail because frontend/features/admin/i18n/en.tsx and ja.tsx are now 1504 lines (limit 1500). Move the new strings into an existing/new scoped translation module rather than growing the capped catalogs, then rerun all required checks.
- [P2] Operator guidance does not match the fallback contract. The UI says a blank value always uses 8/32 MiB, but the implementation falls back to the environment-configured TOKENHUB_MAX_* values, which may differ. Update the Chinese keys and English/Japanese entries accordingly. Also synchronize the English, Simplified Chinese, and Japanese deployment docs with runtime override precedence, the approximately 10-second propagation window, the 512 MiB ceiling, and the need to keep reverse-proxy limits aligned.
The focused backend runtime-limit tests pass. No approval while the cache behavior, documentation, and required checks remain unresolved.
046883a to
83b1ee2
Compare
|
@astaxie 已按你的要求处理:
CI 现状: 所以本 PR 已就绪,等 main 侧那个测试修复(或若属 flaky 则重跑)即可。其余细节欢迎指出。 |
83b1ee2 to
48f47b2
Compare
|
@astaxie 已按你说的处理并重新推送(head
CI: |
|
@astaxie 顺带反馈一个 main 侧的 bug(与本 PR 无关,但导致本 PR 的 Backend CI 跟着红):
所以这不是 flake,是可复现的 main 侧 bug(测试未适配 c1030f5 的 upstream-refresh 行为)。 等 main 侧修好后我会 rebase 到最新 main 并重新 push。在此之前本 PR 已就绪(其余 4 项全绿,标题祈使句,翻译已补齐)。 |
astaxie
left a comment
There was a problem hiding this comment.
Re-reviewed the complete current diff at 48f47b23. This head is still not merge-ready because the substantive findings from the previous review remain in the current code:
- The advertised 10-second cache is still never fresh.
readEffectiveBodyLimitsstill returns without assigningrefreshedAt, so everyeffective*RequestLimitcall treats the snapshot as expired and readssettingsagain. The new tests callreadEffectiveBodyLimitsdirectly or make only one effective-limit lookup; they do not count repeated settings reads or exercise TTL expiry. Please set the timestamp when building the snapshot and add the requested cache-read/expiry regression. - Operator guidance still contradicts the fallback contract. The System Settings help text still says blank values use fixed 8/32 MiB defaults, although the code falls back to the deployment-specific
TOKENHUB_MAX_*values. There are still no English, Simplified Chinese, or Japanese deployment-document updates for override precedence, the approximately 10-second propagation window, the 512 MiB ceiling, and reverse-proxy alignment. - Required CI remains red. The current Backend job fails both
TestAdminProviderCatalogRoutesPreserveRefreshQueryandTestUsageBreakdownQueryCountDoesNotGrowWithRecords. I reproduced the provider-catalog failure on currentorigin/main, so that part is a base-branch problem rather than a body-limit regression, but the repository rule still requires all required checks to be green before approval.
Focused runtime-limit tests, the UI translation gate, source-line gate, and git diff --check pass. The title/conflict/source-line issues are fixed, but approval must wait for the two unchanged implementation/documentation blockers and a green required CI run.
(TOKENHUB_MAX_JSON_REQUEST_BYTES / TOKENHUB_MAX_MULTIMODAL_REQUEST_BYTES)with 413 / http.MaxBytesReader streaming / separate regular and multimodaltiers / a 512 MiB ceiling. Changing them still required a restart, so anoperator could not raise the cap when a workload (Codex computer-use imagerecognition, long-context vision) hit the default mid-flight withoutdraining the gateway.This adds a runtime-adjustable layer on top of astaxie#144, without touching itsdecode contract. The effective limit for each tier is read from thecfg_gateway system setting (max_json_request_bytes / max_multimodal_request_bytes)and cached with a short TTL via an atomic snapshot, so the hot path does notquery the store per request; admin edits take effect for new requests withinthe TTL and never need a restart. A missing, malformed, or non-positive valuefalls back to the env default, so the env stays the deployment-time defaultand 512 MiB (maxConfigurableRequestBytes) stays the hard ceiling for both.decodeJSON / decodeJSONOptional use effectiveJSONRequestLimit(); the eightmultimodal call sites (gateway, anthropic, gemini, playground) useeffectiveMultimodalRequestLimit(). astaxie#144's 413 payload_too_large,http.MaxBytesReader streaming, two-tier limits, single-value check, anderrEmptyRequestBody / decodeJSONOptional semantics are unchanged.cfg_gateway is deliberately not seeded with the two fields, so a freshdeployment preserves any operator-customized env value until an admin setsan override; the System Settings form leaves the fields empty (placeholderonly) so saving the page does not silently write a number that overrides theenv default. The two new labels and help strings are added to en.tsx andja.tsx so non-Chinese locales do not fall back to Chinese.
48f47b2 to
7a7ea66
Compare
|
@astaxie 已修好你提的两个点(head P1 缓存失效: P2 帮助文案与回退契约矛盾:
CI 随 push 重新触发中。 |
astaxie
left a comment
There was a problem hiding this comment.
Re-reviewed the complete current diff at exact head 7a7ea66. The prior timestamp and operator-guidance findings are fixed: the focused body-limit tests, go vet, three-language documentation and UI translation gates, source-line gate, and git diff checks pass.
[P2] The advertised approximately 10-second propagation bound is still not guaranteed under concurrent expiry. currentBodyLimits performs an unsynchronized load, settings read, and store. Two expired readers can overlap; a slow read that captured the old cfg_gateway value before an admin update can store that old snapshot after another reader has already stored the new value, restarting the TTL with stale limits. Please serialize refreshes, version the snapshot, or invalidate the cache on the cfg_gateway update path, and add a deterministic overlapping-refresh regression.
The PR also remains blocked by missing GitHub checks and content conflicts with current main in backend/internal/server/http.go, the three deployment guides, and frontend/features/admin/resources/settings-config.tsx. Please reconcile current main, rerun the full applicable checks, and request another review.

Summary
decodeJSON's request body limits were made env-configurable by #144 (merged):TOKENHUB_MAX_JSON_REQUEST_BYTES/TOKENHUB_MAX_MULTIMODAL_REQUEST_BYTES, 413payload_too_large,http.MaxBytesReaderstreaming, separate regular and multimodal tiers, 512 MiB ceiling. Changing them still required a restart, so when a workload (Codex computer-use image recognition, long-context vision) hit the default mid-flight, an operator could not raise the cap without draining the gateway.This PR is a follow-up that adds a runtime-adjustable layer on top of #144, without touching its decode contract: an admin can raise either limit from the System Settings page and it takes effect for new requests within ~10 s, no restart. It is not a replacement — #144's env path, 413, streaming, two tiers, and ceiling all stand; this only adds an in-memory override sourced from the
cfg_gatewaysystem setting.(I saw #144 land while the earlier version of this PR was waiting on review feedback; this is a rebuild onto current main that builds on #144 rather than duplicating it.)
Related Issue
Supplements #144 (which closed #125).
Changes
http_transport.go:effectiveJSONRequestLimit()/effectiveMultimodalRequestLimit()return the effective cap.bodyLimitCache(atomic.Pointer snapshot,bodyLimitTTL = 10s);currentBodyLimits()serves the cache and refreshes viareadEffectiveBodyLimits()on expiry — the hot path does not query the store per request.readEffectiveBodyLimits()defaults tos.config.Max*RequestBytes(the env values from feat(server): enforce configurable JSON request body limits #144), then readscfg_gateway.max_json_request_bytes/max_multimodal_request_bytes(reusing the existingint64Fieldhelper). A set, positive value overrides the env default; a missing / malformed / non-positive value falls back to the env default; both are clamped tomaxConfigurableRequestBytes(512 MiB).apiKeyGenerationConfig:cfg_gatewayis preferred, the first active settings record is a fallback.decodeJSON/decodeJSONOptionaluseeffectiveJSONRequestLimit(); the eight multimodal call sites (gateway_http,anthropic_messages,gemini_native_http,playground_stream) useeffectiveMultimodalRequestLimit().http.go:Servergains abodyLimits bodyLimitCachefield.seed.go: deliberately not seeded with the two fields, so a fresh deployment preserves an operator-customized env value until an admin sets an override.settings-config.tsx), left empty bydefaultFormValues(placeholder only) so saving the page does not silently write a number that overrides the env default. The two labels and help strings are added toen.tsxandja.tsxso non-Chinese locales do not fall back to Chinese.body_limit_runtime_test.gocovers override honored / clamp-to-ceiling / bad-value fallback / partial override / cfg_gateway preference, plus an end-to-end that a raised runtime limit accepts a previously-over-limit body and still 413s beyond it, and that the multimodal tier is independent of the JSON tier.#144's
decodeJSONLimit(413,http.MaxBytesReader, single-value check,errEmptyRequestBody/decodeJSONOptionalsemantics) is unchanged.Type of Change
Verification
gofmt -lon changed Go files — clean.go vet ./...(CGO_ENABLED=1) — clean.go build ./...— clean.go test ./internal/server/— new tests pass; the only failures on this host are pre-existing environment ones (Postgres/multi-instance, sqlite-backup,version_updatenetwork/bin/tokenhub) that fail identically on the clean base commit (25c481a) with these changes stashed, plus one analytics-snapshot test that is flaky in the full suite but passes in isolation; none are caused by this change.-race.npm run typecheckandnpm run build— clean.node tools/check-ui-translations.mjs(passed, 0 new literal keys),node tools/check-env-contract.mjs(passed, 55 backend variables; no env var introduced),node --test tools/*.test.mjs(116 passed / 0 failed),node tools/check-source-lines.mjs(passed).git diff --check origin/main HEAD— passes.Compatibility, Security, and Operations
cfg_gatewayoverride, the effective limit equals the feat(server): enforce configurable JSON request body limits #144 env default.maxConfigurableRequestBytes), so an operator cannot raise it beyond the budget feat(server): enforce configurable JSON request body limits #144 already set.cfg_gatewaysettings resourceFieldsmap).Checklist
.envfiles, databases, backups, or runtime logs are included. (frontend/next-env.d.tsis not committed — it is a Next.js build artifact.)start.sh, and deployment documentation where applicable. — N/A; no environment variable introduced; the override is a DB-backed system setting, and feat(server): enforce configurable JSON request body limits #144 already synced the env vars.en.tsxandja.tsx(the ZH literals are the source). The UI translation gate passes with 0 new literal keys; the dynamictx(field.*)keys are covered manually per the earlier review feedback.data/model-catalog.yamlremains tracked and catalog changes were reviewed where applicable. — N/A.git diff --checkpasses.