Add governor-based channel selection and fork validation lab - #4183
Closed
ChengQian1129 wants to merge 16 commits into
Closed
Add governor-based channel selection and fork validation lab#4183ChengQian1129 wants to merge 16 commits into
ChengQian1129 wants to merge 16 commits into
Conversation
added 16 commits
April 9, 2026 22:59
Contributor
WalkthroughThis pull request introduces a comprehensive "governor" rate-limiting and concurrency control system integrated across relay/task request handling, channel selection, and storage backends. It includes GitHub Actions workflows for CI/CD, a lab environment with Docker Compose setup, a Redis-backed governor service with Lua scripts, refactored channel multi-key selection, and supporting infrastructure like bootstrap tooling and verification scripts. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
relay/helper/stream_scanner.go-43-46 (1)
43-46:⚠️ Potential issue | 🟡 MinorUpdate the stale initialization comment.
Line 43 says StreamStatus is initialized unconditionally, but Lines 44-46 now do conditional initialization. Please align the comment with the actual behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/helper/stream_scanner.go` around lines 43 - 46, The comment above the StreamStatus initialization is stale: update the comment to reflect conditional initialization rather than "unconditional" creation. Edit the comment near the block that checks info.StreamStatus and calls relaycommon.NewStreamStatus() so it accurately states that StreamStatus is created only when info.StreamStatus is nil (e.g., "Initialize StreamStatus if absent" or similar).relay/channel/claude/relay-claude.go-359-362 (1)
359-362:⚠️ Potential issue | 🟡 MinorBug: Index computed on trimmed string but used on original string.
The
dotindex is computed fromstrings.TrimSpace(file.FileName)but then applied to slice the originalfile.FileName. If the filename has leading whitespace, the index will be misaligned and extract an incorrect extension.🐛 Proposed fix
- providedMimeType := "" - if dot := strings.LastIndex(strings.TrimSpace(file.FileName), "."); dot != -1 && dot+1 < len(file.FileName) { - providedMimeType = service.GetMimeTypeByExtension(file.FileName[dot+1:]) - } + providedMimeType := "" + trimmedName := strings.TrimSpace(file.FileName) + if dot := strings.LastIndex(trimmedName, "."); dot != -1 && dot+1 < len(trimmedName) { + providedMimeType = service.GetMimeTypeByExtension(trimmedName[dot+1:]) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/claude/relay-claude.go` around lines 359 - 362, The code computes dot using strings.LastIndex on strings.TrimSpace(file.FileName) but then slices the untrimmed file.FileName, which can misalign the index for names with leading/trailing spaces; fix by assigning the trimmed name to a local var (e.g., trimmedName := strings.TrimSpace(file.FileName)), compute dot off trimmedName, and call service.GetMimeTypeByExtension on the substring/slice of trimmedName (or use path.Ext on trimmedName) so providedMimeType is derived from the trimmed filename; update references to use trimmedName and keep the existing bounds checks around dot and length..github/workflows/fork-ghcr-image.yml-56-58 (1)
56-58:⚠️ Potential issue | 🟡 MinorGroup output redirects to prevent ShellCheck SC2129 warning.
Lines 56–58 use repeated
>> "${GITHUB_OUTPUT}"writes; group them into a single redirect to optimize file I/O and satisfy linters.Suggested diff
- echo "repo=${repo}" >> "${GITHUB_OUTPUT}" - echo "channel=${channel}" >> "${GITHUB_OUTPUT}" - echo "version=${version}" >> "${GITHUB_OUTPUT}" + { + echo "repo=${repo}" + echo "channel=${channel}" + echo "version=${version}" + } >> "${GITHUB_OUTPUT}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/fork-ghcr-image.yml around lines 56 - 58, Group the three echo writes to GITHUB_OUTPUT into a single redirected block to avoid repeated appends and satisfy ShellCheck SC2129; replace the separate echo "repo=...", echo "channel=...", echo "version=..." statements with a grouped command (e.g., using { echo "repo=${repo}"; echo "channel=${channel}"; echo "version=${version}"; } >> "${GITHUB_OUTPUT}") so all three values are written in one I/O operation.deploy/lab/stop-wsl-lab.sh-78-82 (1)
78-82:⚠️ Potential issue | 🟡 MinorMake fallback app shutdown deterministic.
At lines 78-82, fallback
new-apishutdown sends SIGTERM but does not wait for graceful exit or escalate to SIGKILL, risking orphan processes. This is inconsistent with both the PID-file shutdown method (lines 36-72) and the fallback Redis shutdown (lines 85-97), which both follow a wait-and-force-kill pattern.Proposed fix
fallback_app_pid="$(find_app_pid)" if [[ -n "${fallback_app_pid}" ]]; then - kill "${fallback_app_pid}" 2>/dev/null || true - echo "new-api: stopped fallback pid ${fallback_app_pid}" + kill "${fallback_app_pid}" 2>/dev/null || true + for _ in $(seq 1 20); do + if ! kill -0 "${fallback_app_pid}" 2>/dev/null; then + echo "new-api: stopped fallback pid ${fallback_app_pid}" + break + fi + sleep 1 + done + if kill -0 "${fallback_app_pid}" 2>/dev/null; then + kill -9 "${fallback_app_pid}" 2>/dev/null || true + echo "new-api: force stopped fallback pid ${fallback_app_pid}" + fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/lab/stop-wsl-lab.sh` around lines 78 - 82, The fallback shutdown currently finds fallback_app_pid via find_app_pid and sends kill (SIGTERM) but doesn't wait or force-kill; update the block that uses fallback_app_pid to mirror the PID-file shutdown logic: after kill "${fallback_app_pid}" send SIGTERM, poll/wait up to the same timeout used elsewhere for the process to exit (checking the pid still exists), and if it remains, send kill -9 (SIGKILL) and confirm termination; reference the variables and functions fallback_app_pid and find_app_pid and reuse the same wait/sleep interval and timeout values used in the PID-file shutdown code path so behavior is consistent with the Redis fallback shutdown.deploy/lab/start-wsl-mock-openai.sh-17-23 (1)
17-23:⚠️ Potential issue | 🟡 MinorVerify PID ownership before treating service as already running.
kill -0only proves the PID exists; it does not verify the process is the mock service. A reused PID can cause startup to skip incorrectly. Add a process argument check to confirm the stored PID still belongs to the intended service.Proposed fix
if [[ -f "${PID_FILE}" ]]; then existing_pid="$(tr -d '\r\n' < "${PID_FILE}")" - if [[ -n "${existing_pid}" ]] && kill -0 "${existing_pid}" 2>/dev/null; then + if [[ -n "${existing_pid}" ]] \ + && kill -0 "${existing_pid}" 2>/dev/null \ + && ps -p "${existing_pid}" -o args= | grep -Fq "run-wsl-mock-openai.sh"; then echo "mock OpenAI upstream is already running with pid ${existing_pid}" exit 0 fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/lab/start-wsl-mock-openai.sh` around lines 17 - 23, The current startup check uses kill -0 on ${existing_pid} which only verifies the PID exists; update the logic around PID_FILE/existing_pid in start-wsl-mock-openai.sh to also verify the process command line belongs to the mock service before deciding it's already running: use ps (e.g., ps -p "${existing_pid}" -o args=) to read the process args for the PID and check for an expected identifier (the mock process name or script string used when launching the mock) and only exit 0 if that identifier is present; if the identifier does not match or ps returns empty, treat the PID as stale (remove the PID_FILE or ignore it) and continue startup.deploy/lab/run-wsl-lab.sh-47-49 (1)
47-49:⚠️ Potential issue | 🟡 MinorValidate PID-file process identity before skipping Redis start.
At line 47,
kill -0alone is insufficient; a reused PID can incorrectly report Redis as running. Add command-line validation to ensure the process is actually redis-server on the correct port.Proposed fix
-if [[ -f "${REDIS_PID_FILE}" ]] && kill -0 "$(cat "${REDIS_PID_FILE}")" 2>/dev/null; then +if [[ -f "${REDIS_PID_FILE}" ]] \ + && kill -0 "$(cat "${REDIS_PID_FILE}")" 2>/dev/null \ + && ps -p "$(cat "${REDIS_PID_FILE}")" -o args= | grep -Eq "redis-server .*--port ${REDIS_PORT}\b"; then echo "Redis already running on port ${REDIS_PORT}" else🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/lab/run-wsl-lab.sh` around lines 47 - 49, The current PID-file check (kill -0 on REDIS_PID_FILE) can falsely accept a reused PID; update the startup guard to validate the PID's command and port before skipping Redis start: read PID from REDIS_PID_FILE, verify the process command line is redis-server (e.g., via /proc/<PID>/cmdline or ps -p <PID> -o comm=) and confirm it is bound to REDIS_PORT (e.g., via ss/netstat checking LISTEN sockets for that PID/port); only print "Redis already running on port ${REDIS_PORT}" and skip startup if both the command matches redis-server and the port is bound by that PID, otherwise treat the PID file as stale and start Redis.controller/relay.go-253-256 (1)
253-256:⚠️ Potential issue | 🟡 MinorCorrupted comment text.
The comments contain garbled Chinese text (encoding corruption). Consider restoring the original comments or removing them if they're no longer relevant.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controller/relay.go` around lines 253 - 256, The inline comments next to Subprotocols and CheckOrigin contain corrupted/garbled text; locate the WebSocket upgrader configuration (the Subprotocols: []string{"realtime"} line and the CheckOrigin: func(r *http.Request) bool { ... } block) and either restore the original human-readable comments (in English or valid UTF-8 Chinese) or remove them entirely; ensure any retained comments clearly describe purpose (e.g., supported subprotocols and why CheckOrigin returns true) and are encoded as valid UTF-8 so future reviewers can read them.model/channel_cache.go-157-157 (1)
157-157:⚠️ Potential issue | 🟡 MinorCorrupted error message strings.
The error messages at these lines contain garbled text (encoding corruption). Compare to the working messages in
GetRandomSatisfiedChannel(e.g., line 257:"数据库一致性错误，渠道# %d 不存在，请联系管理员修复").Suggested fix
- return nil, fmt.Errorf("鏁版嵁搴撲竴鑷存€ч敊璇紝娓犻亾# %d 涓嶅瓨鍦紝璇疯仈绯荤鐞嗗憳淇", channels[0]) + return nil, fmt.Errorf("数据库一致性错误，渠道# %d 不存在，请联系管理员修复", channels[0])Apply similar fixes to lines 165 and 192.
Also applies to: 165-165, 192-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/channel_cache.go` at line 157, Replace the garbled error-message strings with the correct Chinese message used in GetRandomSatisfiedChannel: change the fmt.Errorf(...) calls that currently contain corrupted text (the one returning fmt.Errorf with channels[0]) to use "数据库一致性错误，渠道# %d 不存在，请联系管理员修复" (format with the channel id). Apply the same replacement for the other two corrupted fmt.Errorf occurrences noted (the ones at the other return sites on the same pattern) so all three error returns use the identical, uncorrupted message.
🧹 Nitpick comments (13)
relay/channel/claude/relay-claude.go (1)
393-404: Non-image MIME types silently treated as images in default case.The default branch assumes any MIME type that isn't
text/*,application/pdf, empty, orapplication/octet-streamis an image. This could cause issues with audio (audio/*), video (video/*), or other unsupported binary formats being incorrectly sent as images to Claude.Consider either explicitly whitelisting supported image MIME types or logging/erroring for unsupported types.
💡 Option: Whitelist known image types
case mimeType == "" || mimeType == "application/octet-stream": continue - default: + case strings.HasPrefix(mimeType, "image/"): claudeMediaMessages = append(claudeMediaMessages, dto.ClaudeMediaMessage{ Type: "image", Source: &dto.ClaudeMessageSource{ Type: "base64", MediaType: mimeType, Data: base64Data, }, }) + default: + // Skip unsupported MIME types (audio, video, etc.) + continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/claude/relay-claude.go` around lines 393 - 404, The switch's default branch incorrectly treats all non-text/pdf/octet MIME types as images; update the handler around mimeType, base64Data and claudeMediaMessages to explicitly whitelist image MIME types (e.g., image/png, image/jpeg, image/gif, image/webp, image/svg+xml, image/bmp, image/tiff or any mimeType that matches prefix "image/") before appending dto.ClaudeMediaMessage with dto.ClaudeMessageSource, and for any unsupported mimeType skip the attachment and emit a warning log (use the existing logger in this file) instead of silently treating it as an image.deploy/lab/.env.example (1)
1-2: Optional: reorder keys to satisfy dotenv-linter ordering.If dotenv-linter is CI-gated, place
HOST_HTTP_PORTbeforeNEW_API_IMAGEto avoid warning churn.Minimal reorder
-NEW_API_IMAGE=ghcr.io/your-github-username/new-api:lab HOST_HTTP_PORT=3000 +NEW_API_IMAGE=ghcr.io/your-github-username/new-api:lab🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/lab/.env.example` around lines 1 - 2, Reorder the two environment variable entries in deploy/lab/.env.example so HOST_HTTP_PORT appears before NEW_API_IMAGE to satisfy dotenv-linter ordering; specifically move the HOST_HTTP_PORT=3000 line above NEW_API_IMAGE=ghcr.io/your-github-username/new-api:lab so the keys are alphabetically/linters-expected ordered.deploy/lab/cmd/mock-openai-upstream/main.go (1)
26-29: Set explicit HTTP server timeouts.
http.Servercurrently has no timeout guards. AddReadHeaderTimeout(at least) and preferablyReadTimeout/WriteTimeout/IdleTimeoutto avoid slow-client exhaustion.Suggested hardening patch
server := &http.Server{ Addr: *listenAddr, Handler: handler, + ReadHeaderTimeout: 5 * time.Second, + ReadTimeout: 15 * time.Second, + WriteTimeout: 30 * time.Second, + IdleTimeout: 60 * time.Second, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/lab/cmd/mock-openai-upstream/main.go` around lines 26 - 29, The http.Server instantiation for variable server currently lacks timeouts; update the struct literal used to create server (the &http.Server{...} block where Addr: *listenAddr and Handler: handler are set) to include ReadHeaderTimeout (at least), and also add sensible ReadTimeout, WriteTimeout and IdleTimeout values (e.g. small seconds for read header/read, moderate for write/idle) so slow clients cannot exhaust resources; ensure the new timeout fields are added alongside Addr and Handler in the same server variable initialization.docs/installation/fork-governor-lab.md (1)
176-182: Add a subsection title before Docker Compose startup commands.This block reads like a flow switch from WSL to Docker. A small header (e.g.,
### Docker Compose 启动) would reduce ambiguity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/installation/fork-governor-lab.md` around lines 176 - 182, Insert a short subsection header immediately above the bash block that begins with "cd deploy/lab" (e.g., add "### Docker Compose 启动") so the Docker Compose startup commands are clearly labeled; update the markdown around that code fence containing the three commands to include this header and ensure it precedes the ```bash block.relay/relay_task.go (1)
97-100: Add a guard after context readback to fail fast on incomplete channel context.Right now, empty/zero values from context are accepted silently. A small validation here will prevent downstream failures with less actionable errors.
Suggested hardening diff
info.ChannelBaseUrl = common.GetContextKeyString(c, constant.ContextKeyChannelBaseUrl) info.ChannelId = common.GetContextKeyInt(c, constant.ContextKeyChannelId) info.ChannelType = common.GetContextKeyInt(c, constant.ContextKeyChannelType) info.ApiKey = common.GetContextKeyString(c, constant.ContextKeyChannelKey) + if info.ChannelId == 0 || info.ApiKey == "" { + return service.TaskErrorWrapperLocal( + errors.New("selected channel context is incomplete"), + "channel_context_invalid", + http.StatusInternalServerError, + ) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/relay_task.go` around lines 97 - 100, After reading channel values into info (using common.GetContextKeyString/GetContextKeyInt for constant.ContextKeyChannelBaseUrl, ContextKeyChannelId, ContextKeyChannelType, ContextKeyChannelKey), add a guard that validates these required fields (non-empty ChannelBaseUrl and ApiKey, non-zero ChannelId and ChannelType) and fail fast by returning an error (or HTTP 400) from the enclosing handler/function if any are missing; update the code paths that use info to rely on this early check so downstream logic (references to info.ChannelBaseUrl, info.ChannelId, info.ChannelType, info.ApiKey) will never see empty/zero values.service/channel_select_governor_test.go (1)
164-167: Cover the service wrapper on the re-enable assertion.This regression is exposed through
CacheGetRandomSatisfiedChannel, but the final assertion only callsmodel.GetRandomSatisfiedChannel. Add one post-reenable service-level assertion so the test exercises the exact cache-backed selection path this PR changes.♻️ Proposed test addition
reenabled, err := model.GetRandomSatisfiedChannel("default", "gpt-4o", 0) require.NoError(t, err) require.NotNil(t, reenabled) require.Equal(t, channelRecord.Id, reenabled.Id) + + rec := httptest.NewRecorder() + ctx, _ := gin.CreateTestContext(rec) + selectedViaService, _, err := CacheGetRandomSatisfiedChannel(&RetryParam{ + Ctx: ctx, + TokenGroup: "default", + ModelName: "gpt-4o", + Retry: common.GetPointer(0), + }) + require.NoError(t, err) + require.NotNil(t, selectedViaService) + require.Equal(t, channelRecord.Id, selectedViaService.Id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/channel_select_governor_test.go` around lines 164 - 167, After re-enabling the channel via model.GetRandomSatisfiedChannel, add a service-level assertion that exercises the cache-backed selection path by calling CacheGetRandomSatisfiedChannel("default", "gpt-4o", 0) (or the service wrapper that invokes that cache path) and assert the returned channel ID equals channelRecord.Id; this ensures the test covers the CacheGetRandomSatisfiedChannel/service wrapper behavior in addition to the model-level call.deploy/lab/cmd/governor-lab-bootstrap/main.go (1)
45-59: Add a timeout around the bootstrap call.
client.Bootstrapperforms HTTP setup work but runs undercontext.Background(), which has no timeout. If the server becomes unresponsive, the lab command can hang indefinitely.♻️ Proposed fix
import ( "context" "flag" "log" "os" + "time" @@ - result, err := client.Bootstrap(context.Background(), governorlab.BootstrapConfig{ + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + + result, err := client.Bootstrap(ctx, governorlab.BootstrapConfig{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/lab/cmd/governor-lab-bootstrap/main.go` around lines 45 - 59, The Bootstrap call uses context.Background() so it can hang indefinitely; replace it by creating a cancellable context with a timeout (e.g., ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)) and defer cancel(), then pass ctx to client.Bootstrap instead of context.Background(); update imports to include time and ensure any surrounding error handling for client.Bootstrap(result, err) remains unchanged while respecting the timeout behavior.service/governor/manager_test.go (2)
49-57: Redundant return path in test double.Lines 53-56 check
s.allowRPMbut both branches returntrue, nil. The condition has no effect.Simplified implementation
func (s *managerTestStore) AllowChannelRPM(_ context.Context, _ int, _ int64) (bool, error) { if s.allowRPMErr != nil { return false, s.allowRPMErr } - if s.allowRPM { - return true, nil - } - return true, nil + return s.allowRPM || true, nil // or simply: return true, nil }Or if the intent was to return
s.allowRPM:- if s.allowRPM { - return true, nil - } - return true, nil + return s.allowRPM, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/governor/manager_test.go` around lines 49 - 57, The test double AllowChannelRPM has a redundant branch: when s.allowRPMErr is nil it always returns true despite checking s.allowRPM; update the implementation in AllowChannelRPM to return the actual flag (s.allowRPM) when there is no error (i.e., return s.allowRPM, nil) and remove the unnecessary if/else so s.allowRPMErr and s.allowRPM are used correctly.
196-196: Simplify double negation assertion.The assertion
require.False(t, getAttemptFromContext(c) != nil)uses a double negation. Consider usingrequire.Nilfor clarity.Suggested fix
- require.False(t, getAttemptFromContext(c) != nil) + require.Nil(t, getAttemptFromContext(c))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/governor/manager_test.go` at line 196, Replace the double-negation assertion require.False(t, getAttemptFromContext(c) != nil) with a direct nil check by calling require.Nil(t, getAttemptFromContext(c)); update the test in manager_test.go where getAttemptFromContext(c) is asserted to be absent so the intent is clearer and the assertion reads directly that the result is nil.deploy/lab/governorlab/bootstrap_test.go (1)
13-32: Consider using subtests for better test isolation and reporting.Using
t.Runfor each test case provides better isolation and clearer failure output.Suggested refactor
func TestRequiresPrivateIPAccess(t *testing.T) { t.Parallel() cases := []struct { + name string baseURL string want bool }{ - {baseURL: "http://127.0.0.1:8080", want: true}, - {baseURL: "http://localhost:8080", want: true}, - {baseURL: "http://172.22.240.24:8080", want: true}, - {baseURL: "https://api.openai.com", want: false}, + {name: "loopback", baseURL: "http://127.0.0.1:8080", want: true}, + {name: "localhost", baseURL: "http://localhost:8080", want: true}, + {name: "private_ip", baseURL: "http://172.22.240.24:8080", want: true}, + {name: "public_https", baseURL: "https://api.openai.com", want: false}, } for _, tc := range cases { - got := RequiresPrivateIPAccess(tc.baseURL) - if got != tc.want { - t.Fatalf("RequiresPrivateIPAccess(%q) = %v, want %v", tc.baseURL, got, tc.want) - } + t.Run(tc.name, func(t *testing.T) { + got := RequiresPrivateIPAccess(tc.baseURL) + if got != tc.want { + t.Errorf("RequiresPrivateIPAccess(%q) = %v, want %v", tc.baseURL, got, tc.want) + } + }) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/lab/governorlab/bootstrap_test.go` around lines 13 - 32, Refactor TestRequiresPrivateIPAccess to run each table case as a subtest using t.Run; inside the loop call tc := tc to capture the range variable, then use t.Run(tc.baseURL, func(t *testing.T) { t.Parallel(); got := RequiresPrivateIPAccess(tc.baseURL); if got != tc.want { t.Fatalf("RequiresPrivateIPAccess(%q) = %v, want %v", tc.baseURL, got, tc.want) } }); this gives better isolation and clearer failure output while still testing the RequiresPrivateIPAccess function.middleware/channel_selection_governor.go (1)
13-13: Unused constantgovernorSelectionRejectedMessage.This constant is declared but not referenced anywhere in this file. Consider removing it or using it in error messages (e.g., lines 45-49 or 55-59) for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/channel_selection_governor.go` at line 13, The constant governorSelectionRejectedMessage is declared but unused; either remove it or replace the hard-coded rejection strings in the channel selection error paths with this constant. Locate the places that currently return or log "all candidate channels are cooling or saturated" (the selection rejection/error return paths in this file) and use governorSelectionRejectedMessage instead so messages are consistent, or delete the constant if you prefer the inline text.deploy/lab/start-wsl-lab.sh (1)
19-20: Confusing variable fallback chain with potential self-reference.The expressions
${APP_PORT:-${LAB_APP_PORT_DEFAULT}}nested inside another APP_PORT assignment create a confusing fallback chain. IfAPP_PORTis already set in the environment, the outer${PORT:-...}fallback won't be evaluated. Consider simplifying:Suggested simplification
-APP_PORT="${PORT:-${LAB_APP_PORT:-${APP_PORT:-${LAB_APP_PORT_DEFAULT}}}}" -REDIS_PORT="${REDIS_PORT:-${LAB_REDIS_PORT:-${REDIS_PORT:-${LAB_REDIS_PORT_DEFAULT}}}}" +APP_PORT="${PORT:-${LAB_APP_PORT:-${LAB_APP_PORT_DEFAULT}}}" +REDIS_PORT="${REDIS_PORT:-${LAB_REDIS_PORT:-${LAB_REDIS_PORT_DEFAULT}}}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deploy/lab/start-wsl-lab.sh` around lines 19 - 20, Replace the self-referential fallback chains for APP_PORT and REDIS_PORT with a clear, non-circular precedence list: ensure APP_PORT is assigned from PORT first, then LAB_APP_PORT, then LAB_APP_PORT_DEFAULT (do not reference APP_PORT on the right-hand side), and ensure REDIS_PORT is assigned from REDIS_PORT first, then LAB_REDIS_PORT, then LAB_REDIS_PORT_DEFAULT (do not reference REDIS_PORT on the right-hand side); update the assignments that currently mention APP_PORT or REDIS_PORT within their own fallbacks to use only the explicit environment fallbacks (PORT, LAB_APP_PORT, LAB_APP_PORT_DEFAULT and LAB_REDIS_PORT, LAB_REDIS_PORT_DEFAULT) to remove confusion and potential self-reference.service/governor/classifier.go (1)
59-65:parseRetryAfterlacks HTTP-date format support.Per RFC 7231 Section 7.1.3, the
Retry-Afterheader supports both delay-seconds (e.g.,120) and HTTP-date (e.g.,Fri, 31 Dec 1999 23:59:59 GMT) formats. The function currently handles only delay-seconds; HTTP-date values will silently return 0. While delay-seconds is common for rate limiting, consider adding HTTP-date parsing for full RFC compliance or document this intentional limitation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/governor/classifier.go` around lines 59 - 65, Update parseRetryAfter to handle HTTP-date formats in addition to delay-seconds: first attempt the existing strconv.Atoi path for numeric delay-seconds (function: parseRetryAfter), and if that fails, try parsing the value as an HTTP-date using standard Go time layouts (e.g., time.RFC1123 / RFC1123Z and optionally RFC850/ANSIC as fallbacks) to obtain a time.Time; compute the duration as parsedTime.Sub(time.Now()) and return max(parsedDuration, 0) (returning 0 for past dates or parse errors). Ensure the function still trims whitespace and returns a non-negative time.Duration.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deploy/lab/cmd/governor-lab-bootstrap/main.go`:
- Line 5: The file imports and calls the stdlib package encoding/json directly
(e.g. json.Marshal/json.Unmarshal) which violates the repo rule; replace that
import and all direct uses with the project JSON wrapper implemented in
common/json.go (use the wrapper's Marshal/Unmarshal functions exposed by that
package), updating import and any calls in main.go (including the usage around
the code previously using encoding/json) so all serialization/deserialization
goes through the common/json.go helpers.
In `@deploy/lab/governorlab/bootstrap.go`:
- Around line 155-164: The env file is written with world-readable permissions;
update the call that writes the file (the os.WriteFile invocation in
bootstrap.go that writes content created from the lines slice including
GOVERNOR_LAB_API_KEY) to use secret-safe permissions (0600 / 0o600) instead of
0644 so only the owner can read/write the file; locate the code that constructs
lines (uses quoteEnvValue and result) and change the file mode argument passed
to os.WriteFile(path, []byte(content), ...) to 0o600.
- Around line 391-435: In Client.doJSON, replace direct encoding/json usage with
the repo wrappers: use common.Marshal(payload) instead of json.Marshal(payload)
when building the request body; use common.DecodeJson(resp.Body, &envelope)
instead of json.NewDecoder(resp.Body).Decode(&envelope) to parse the API
envelope; and use common.Unmarshal(envelope.Data, out) instead of
json.Unmarshal(envelope.Data, out) to unmarshal the envelope.Data into out —
keep existing error handling, nil checks for out/envelope.Data, and the
apiEnvelope/envelope variable names intact.
In `@deploy/lab/governorlab/mock.go`:
- Around line 125-136: In writeMockStreamResponse, check for http.Flusher
support before sending any headers or calling w.WriteHeader; if the type
assertion to flusher fails, call writeMockJSON with the 500 status and return
without having written a 200. After confirming flusher (flusher, ok :=
w.(http.Flusher)), then set the Content-Type/Cache-Control/Connection headers
and call w.WriteHeader(http.StatusOK) before streaming; this ensures
writeMockJSON can send the correct error status when streaming is unsupported.
In `@deploy/lab/start-wsl-mock-openai.sh`:
- Line 35: The health-check curl call that probes
http://127.0.0.1:${MOCK_PORT}/healthz can hang indefinitely; update the
conditional that calls curl to include per-request timeouts (e.g.,
--connect-timeout and --max-time) so each probe fails fast and the overall loop
bounded by START_TIMEOUT_SECONDS is deterministic; modify the curl invocation in
the start-wsl-mock-openai.sh health check (the line using curl ...
"http://127.0.0.1:${MOCK_PORT}/healthz") to add appropriate timeout flags
(choose small sensible values such as a short connect timeout and a slightly
larger total max time) so a single slow connection cannot consume the startup
deadline.
In `@deploy/lab/verify-governor.sh`:
- Around line 64-70: The curl call in verify-governor.sh that posts to
"${BASE_URL%/}${API_PATH}" using ${API_KEY}, writing to "${body_file}" and
"${code_file}" needs per-request timeouts to avoid hanging; update that curl
invocation to include connection and overall time limits (e.g.,
--connect-timeout and --max-time) and optionally a small retry count (e.g.,
--retry) so stalled requests fail fast and return a non-zero exit/status to the
caller, ensuring the surrounding governor logic can make progress.
- Around line 54-56: The current heredoc writes MODEL and PROMPT raw into
payload_file which can produce invalid JSON; instead build the JSON payload with
proper escaping (e.g., use a JSON builder like jq or a small python/ruby/json
tool) by passing MODEL and PROMPT as arguments (use --arg or equivalent) and
writing the resulting JSON to payload_file; update the block that writes to
payload_file (the cat/heredoc) to use this safe JSON construction so MODEL and
PROMPT are correctly escaped and embedded into the "model" and
messages[0].content fields.
In `@deploy/lab/verify-wsl-governor-e2e.sh`:
- Around line 16-18: The MOCK_PORT assignment currently falls back to literal
8080 instead of honoring a persisted LAB_MOCK_PORT from session.env; change the
MOCK_PORT initialization to mirror APP_PORT's pattern by checking MOCK_PORT,
then LAB_MOCK_PORT, then a default constant (e.g., MOCK_PORT_DEFAULT or a final
literal) so the persisted port is respected after sourcing SESSION_FILE; update
any derived uses (MOCK_BASE_URL that uses MOCK_PORT on lines 34–35) to rely on
this corrected MOCK_PORT variable.
In `@middleware/distributor.go`:
- Around line 129-155: The affinity marker is being set before the governor is
successfully initialized, causing unusable channels to be cached; move the call
to markChannelAffinitySelection(c, selectGroup, preferred.Id) so it only runs
after setupChannelContext(c, channel, modelRequest.Model) returns nil (i.e.,
inside the success branch where channelContextReady is set), and ensure that
when setupChannelContext returns a GovernorSelectionRejected (and you call
retryParam.ExcludeChannel(channel.Id)) you do not mark affinity; update
references around selectGroup/channel, setupChannelContext,
markChannelAffinitySelection, shouldSkipRetryAfterAffinityFailure and
retryParam.ExcludeChannel accordingly.
In `@model/channel.go`:
- Around line 276-278: CommitSelectedKeyIndex currently calls
channel.SaveChannelInfo() when !common.MemoryCacheEnabled but ignores its error;
instead, capture and handle the error from SaveChannelInfo() inside
CommitSelectedKeyIndex (or propagate it to the caller) so failures prevent
advancing the polling index and request processing; update
CommitSelectedKeyIndex to check the returned error from
channel.SaveChannelInfo(), return or log and return the error appropriately
(preserving existing error semantics), referencing the CommitSelectedKeyIndex
and SaveChannelInfo methods and the MultiKeyPollingIndex state so the caller
doesn't proceed with a stale index.
In `@service/governor/config.go`:
- Around line 65-77: The global storeFactory is mutated by
SetStoreFactoryForTest and read concurrently by manager.go, causing data races;
introduce a package-level sync.RWMutex (e.g., storeFactoryMu) to protect access:
acquire storeFactoryMu.Lock()/Unlock() around the write in
SetStoreFactoryForTest and use storeFactoryMu.RLock()/RUnlock() at every call
site that invokes storeFactory() in manager.go (and any other readers), or
alternatively replace storeFactory with a sync/atomic.Pointer to the function
for lock-free reads/writes; update references to use the chosen synchronization
primitive around storeFactory accesses.
In `@service/governor/manager.go`:
- Around line 192-195: CompleteTaskAttemptFromContext currently only applies
channel-level cooling; if ClassifyTaskError sets decision.CoolKey we must also
apply key-level cooling so the specific bad key is suppressed. In
CompleteTaskAttemptFromContext, after the existing check that calls
store.CoolChannel(requestContext(c), attempt.ChannelID, decision.TTL), add a
similar guarded call when decision.CoolKey && decision.TTL > 0 to call the
store's key-cooling API with the current request context, attempt.ChannelID and
attempt.Key (preserving the channel cooling behavior).
- Around line 259-279: The release path currently uses requestContext(c) which
may be canceled; change stopAndRelease to create and pass a dedicated
non-cancelable cleanup context (e.g. context.WithTimeout(context.Background(),
5*time.Second) with cancel deferred) into releasePreparedAttempt so
ReleaseKeyLease(ctx, ...) always runs for AttemptState cleanup; alternatively,
inside releasePreparedAttempt wrap the call to store.ReleaseKeyLease in a short
background timeout context to ensure the lease release executes even if
requestContext(c) is canceled. Reference: stopAndRelease,
releasePreparedAttempt, AttemptState.StopHeartbeat, AttemptState.LeaseHeld,
AttemptState.ReservationID, and store.ReleaseKeyLease.
- Around line 40-54: The code currently iterates OrderedEnabledKeyIndices() and
uses channel.KeyAt() for all channels, which changes semantics for single-key
channels; update manager.go to preserve raw channel.Key for non-multi channels
by checking channel.ChannelInfo.IsMultiKey (or using
GetNextEnabledKey()/channel.Key directly) before using
OrderedEnabledKeyIndices()/KeyAt(): if the channel is not multi-key, fetch and
return/submit the raw channel.Key (respecting cooling via store.IsKeyCooling for
the single key if needed) instead of parsing via KeyAt(); otherwise continue to
use OrderedEnabledKeyIndices() and KeyAt() for multi-key channels.
---
Minor comments:
In @.github/workflows/fork-ghcr-image.yml:
- Around line 56-58: Group the three echo writes to GITHUB_OUTPUT into a single
redirected block to avoid repeated appends and satisfy ShellCheck SC2129;
replace the separate echo "repo=...", echo "channel=...", echo "version=..."
statements with a grouped command (e.g., using { echo "repo=${repo}"; echo
"channel=${channel}"; echo "version=${version}"; } >> "${GITHUB_OUTPUT}") so all
three values are written in one I/O operation.
In `@controller/relay.go`:
- Around line 253-256: The inline comments next to Subprotocols and CheckOrigin
contain corrupted/garbled text; locate the WebSocket upgrader configuration (the
Subprotocols: []string{"realtime"} line and the CheckOrigin: func(r
*http.Request) bool { ... } block) and either restore the original
human-readable comments (in English or valid UTF-8 Chinese) or remove them
entirely; ensure any retained comments clearly describe purpose (e.g., supported
subprotocols and why CheckOrigin returns true) and are encoded as valid UTF-8 so
future reviewers can read them.
In `@deploy/lab/run-wsl-lab.sh`:
- Around line 47-49: The current PID-file check (kill -0 on REDIS_PID_FILE) can
falsely accept a reused PID; update the startup guard to validate the PID's
command and port before skipping Redis start: read PID from REDIS_PID_FILE,
verify the process command line is redis-server (e.g., via /proc/<PID>/cmdline
or ps -p <PID> -o comm=) and confirm it is bound to REDIS_PORT (e.g., via
ss/netstat checking LISTEN sockets for that PID/port); only print "Redis already
running on port ${REDIS_PORT}" and skip startup if both the command matches
redis-server and the port is bound by that PID, otherwise treat the PID file as
stale and start Redis.
In `@deploy/lab/start-wsl-mock-openai.sh`:
- Around line 17-23: The current startup check uses kill -0 on ${existing_pid}
which only verifies the PID exists; update the logic around
PID_FILE/existing_pid in start-wsl-mock-openai.sh to also verify the process
command line belongs to the mock service before deciding it's already running:
use ps (e.g., ps -p "${existing_pid}" -o args=) to read the process args for the
PID and check for an expected identifier (the mock process name or script string
used when launching the mock) and only exit 0 if that identifier is present; if
the identifier does not match or ps returns empty, treat the PID as stale
(remove the PID_FILE or ignore it) and continue startup.
In `@deploy/lab/stop-wsl-lab.sh`:
- Around line 78-82: The fallback shutdown currently finds fallback_app_pid via
find_app_pid and sends kill (SIGTERM) but doesn't wait or force-kill; update the
block that uses fallback_app_pid to mirror the PID-file shutdown logic: after
kill "${fallback_app_pid}" send SIGTERM, poll/wait up to the same timeout used
elsewhere for the process to exit (checking the pid still exists), and if it
remains, send kill -9 (SIGKILL) and confirm termination; reference the variables
and functions fallback_app_pid and find_app_pid and reuse the same wait/sleep
interval and timeout values used in the PID-file shutdown code path so behavior
is consistent with the Redis fallback shutdown.
In `@model/channel_cache.go`:
- Line 157: Replace the garbled error-message strings with the correct Chinese
message used in GetRandomSatisfiedChannel: change the fmt.Errorf(...) calls that
currently contain corrupted text (the one returning fmt.Errorf with channels[0])
to use "数据库一致性错误，渠道# %d 不存在，请联系管理员修复" (format with the channel id). Apply the
same replacement for the other two corrupted fmt.Errorf occurrences noted (the
ones at the other return sites on the same pattern) so all three error returns
use the identical, uncorrupted message.
In `@relay/channel/claude/relay-claude.go`:
- Around line 359-362: The code computes dot using strings.LastIndex on
strings.TrimSpace(file.FileName) but then slices the untrimmed file.FileName,
which can misalign the index for names with leading/trailing spaces; fix by
assigning the trimmed name to a local var (e.g., trimmedName :=
strings.TrimSpace(file.FileName)), compute dot off trimmedName, and call
service.GetMimeTypeByExtension on the substring/slice of trimmedName (or use
path.Ext on trimmedName) so providedMimeType is derived from the trimmed
filename; update references to use trimmedName and keep the existing bounds
checks around dot and length.
In `@relay/helper/stream_scanner.go`:
- Around line 43-46: The comment above the StreamStatus initialization is stale:
update the comment to reflect conditional initialization rather than
"unconditional" creation. Edit the comment near the block that checks
info.StreamStatus and calls relaycommon.NewStreamStatus() so it accurately
states that StreamStatus is created only when info.StreamStatus is nil (e.g.,
"Initialize StreamStatus if absent" or similar).
---
Nitpick comments:
In `@deploy/lab/.env.example`:
- Around line 1-2: Reorder the two environment variable entries in
deploy/lab/.env.example so HOST_HTTP_PORT appears before NEW_API_IMAGE to
satisfy dotenv-linter ordering; specifically move the HOST_HTTP_PORT=3000 line
above NEW_API_IMAGE=ghcr.io/your-github-username/new-api:lab so the keys are
alphabetically/linters-expected ordered.
In `@deploy/lab/cmd/governor-lab-bootstrap/main.go`:
- Around line 45-59: The Bootstrap call uses context.Background() so it can hang
indefinitely; replace it by creating a cancellable context with a timeout (e.g.,
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)) and
defer cancel(), then pass ctx to client.Bootstrap instead of
context.Background(); update imports to include time and ensure any surrounding
error handling for client.Bootstrap(result, err) remains unchanged while
respecting the timeout behavior.
In `@deploy/lab/cmd/mock-openai-upstream/main.go`:
- Around line 26-29: The http.Server instantiation for variable server currently
lacks timeouts; update the struct literal used to create server (the
&http.Server{...} block where Addr: *listenAddr and Handler: handler are set) to
include ReadHeaderTimeout (at least), and also add sensible ReadTimeout,
WriteTimeout and IdleTimeout values (e.g. small seconds for read header/read,
moderate for write/idle) so slow clients cannot exhaust resources; ensure the
new timeout fields are added alongside Addr and Handler in the same server
variable initialization.
In `@deploy/lab/governorlab/bootstrap_test.go`:
- Around line 13-32: Refactor TestRequiresPrivateIPAccess to run each table case
as a subtest using t.Run; inside the loop call tc := tc to capture the range
variable, then use t.Run(tc.baseURL, func(t *testing.T) { t.Parallel(); got :=
RequiresPrivateIPAccess(tc.baseURL); if got != tc.want {
t.Fatalf("RequiresPrivateIPAccess(%q) = %v, want %v", tc.baseURL, got, tc.want)
} }); this gives better isolation and clearer failure output while still testing
the RequiresPrivateIPAccess function.
In `@deploy/lab/start-wsl-lab.sh`:
- Around line 19-20: Replace the self-referential fallback chains for APP_PORT
and REDIS_PORT with a clear, non-circular precedence list: ensure APP_PORT is
assigned from PORT first, then LAB_APP_PORT, then LAB_APP_PORT_DEFAULT (do not
reference APP_PORT on the right-hand side), and ensure REDIS_PORT is assigned
from REDIS_PORT first, then LAB_REDIS_PORT, then LAB_REDIS_PORT_DEFAULT (do not
reference REDIS_PORT on the right-hand side); update the assignments that
currently mention APP_PORT or REDIS_PORT within their own fallbacks to use only
the explicit environment fallbacks (PORT, LAB_APP_PORT, LAB_APP_PORT_DEFAULT and
LAB_REDIS_PORT, LAB_REDIS_PORT_DEFAULT) to remove confusion and potential
self-reference.
In `@docs/installation/fork-governor-lab.md`:
- Around line 176-182: Insert a short subsection header immediately above the
bash block that begins with "cd deploy/lab" (e.g., add "### Docker Compose 启动")
so the Docker Compose startup commands are clearly labeled; update the markdown
around that code fence containing the three commands to include this header and
ensure it precedes the ```bash block.
In `@middleware/channel_selection_governor.go`:
- Line 13: The constant governorSelectionRejectedMessage is declared but unused;
either remove it or replace the hard-coded rejection strings in the channel
selection error paths with this constant. Locate the places that currently
return or log "all candidate channels are cooling or saturated" (the selection
rejection/error return paths in this file) and use
governorSelectionRejectedMessage instead so messages are consistent, or delete
the constant if you prefer the inline text.
In `@relay/channel/claude/relay-claude.go`:
- Around line 393-404: The switch's default branch incorrectly treats all
non-text/pdf/octet MIME types as images; update the handler around mimeType,
base64Data and claudeMediaMessages to explicitly whitelist image MIME types
(e.g., image/png, image/jpeg, image/gif, image/webp, image/svg+xml, image/bmp,
image/tiff or any mimeType that matches prefix "image/") before appending
dto.ClaudeMediaMessage with dto.ClaudeMessageSource, and for any unsupported
mimeType skip the attachment and emit a warning log (use the existing logger in
this file) instead of silently treating it as an image.
In `@relay/relay_task.go`:
- Around line 97-100: After reading channel values into info (using
common.GetContextKeyString/GetContextKeyInt for
constant.ContextKeyChannelBaseUrl, ContextKeyChannelId, ContextKeyChannelType,
ContextKeyChannelKey), add a guard that validates these required fields
(non-empty ChannelBaseUrl and ApiKey, non-zero ChannelId and ChannelType) and
fail fast by returning an error (or HTTP 400) from the enclosing
handler/function if any are missing; update the code paths that use info to rely
on this early check so downstream logic (references to info.ChannelBaseUrl,
info.ChannelId, info.ChannelType, info.ApiKey) will never see empty/zero values.
In `@service/channel_select_governor_test.go`:
- Around line 164-167: After re-enabling the channel via
model.GetRandomSatisfiedChannel, add a service-level assertion that exercises
the cache-backed selection path by calling
CacheGetRandomSatisfiedChannel("default", "gpt-4o", 0) (or the service wrapper
that invokes that cache path) and assert the returned channel ID equals
channelRecord.Id; this ensures the test covers the
CacheGetRandomSatisfiedChannel/service wrapper behavior in addition to the
model-level call.
In `@service/governor/classifier.go`:
- Around line 59-65: Update parseRetryAfter to handle HTTP-date formats in
addition to delay-seconds: first attempt the existing strconv.Atoi path for
numeric delay-seconds (function: parseRetryAfter), and if that fails, try
parsing the value as an HTTP-date using standard Go time layouts (e.g.,
time.RFC1123 / RFC1123Z and optionally RFC850/ANSIC as fallbacks) to obtain a
time.Time; compute the duration as parsedTime.Sub(time.Now()) and return
max(parsedDuration, 0) (returning 0 for past dates or parse errors). Ensure the
function still trims whitespace and returns a non-negative time.Duration.
In `@service/governor/manager_test.go`:
- Around line 49-57: The test double AllowChannelRPM has a redundant branch:
when s.allowRPMErr is nil it always returns true despite checking s.allowRPM;
update the implementation in AllowChannelRPM to return the actual flag
(s.allowRPM) when there is no error (i.e., return s.allowRPM, nil) and remove
the unnecessary if/else so s.allowRPMErr and s.allowRPM are used correctly.
- Line 196: Replace the double-negation assertion require.False(t,
getAttemptFromContext(c) != nil) with a direct nil check by calling
require.Nil(t, getAttemptFromContext(c)); update the test in manager_test.go
where getAttemptFromContext(c) is asserted to be absent so the intent is clearer
and the assertion reads directly that the result is nil.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92a82bc7-04e1-4af7-9cb0-6b39fb910cdf
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (58)
.github/workflows/fork-ghcr-image.yml.github/workflows/fork-verify.yml.gitignoreconstant/context_key.gocontroller/relay.gocontroller/relay_selection_test.gocontroller/relay_task_error_test.godeploy/lab/.env.exampledeploy/lab/channel-settings.governor.example.jsondeploy/lab/cmd/governor-lab-bootstrap/main.godeploy/lab/cmd/mock-openai-upstream/main.godeploy/lab/compose.ymldeploy/lab/governorlab/bootstrap.godeploy/lab/governorlab/bootstrap_test.godeploy/lab/governorlab/mock.godeploy/lab/governorlab/mock_test.godeploy/lab/run-wsl-lab.shdeploy/lab/run-wsl-mock-openai.shdeploy/lab/start-wsl-lab.shdeploy/lab/start-wsl-mock-openai.shdeploy/lab/status-wsl-lab.shdeploy/lab/stop-wsl-lab.shdeploy/lab/test-stop-wsl-lab.shdeploy/lab/verify-governor.shdeploy/lab/verify-wsl-governor-e2e.shdocs/installation/fork-governor-lab.mddto/channel_settings.godto/channel_settings_test.gogo.modmiddleware/channel_selection_governor.gomiddleware/channel_selection_governor_test.gomiddleware/distributor.gomiddleware/distributor_governor_test.gomodel/ability.gomodel/channel.gomodel/channel_cache.gomodel/channel_governor_test.gomodel/main.gorelay/channel/claude/relay-claude.gorelay/helper/stream_scanner.gorelay/relay_task.goservice/channel_affinity_usage_cache_test.goservice/channel_select.goservice/channel_select_governor_test.goservice/error.goservice/governor/classifier.goservice/governor/classifier_test.goservice/governor/config.goservice/governor/context.goservice/governor/lua/acquire_lease.luaservice/governor/lua/incr_rpm.luaservice/governor/lua/release_lease.luaservice/governor/lua/touch_lease.luaservice/governor/manager.goservice/governor/manager_test.goservice/governor/redis_store.goservice/governor/redis_store_test.gotypes/error.go
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the direct stdlib JSON usage here and inspect the sanctioned wrappers.
sed -n '1,200p' common/json.go
rg -n 'encoding/json|json\.NewEncoder|json\.Marshal|json\.Unmarshal' deploy/lab/cmd/governor-lab-bootstrap/main.goRepository: QuantumNous/new-api
Length of output: 878
Replace direct encoding/json usage with the project wrapper.
Lines 5 and 70 use the standard library directly, violating the repo-wide rule that all JSON operations in Go files must use wrapper functions from common/json.go.
♻️ Proposed fix
import (
"context"
- "encoding/json"
"flag"
"log"
"os"
+ "github.com/QuantumNous/new-api/common"
"github.com/QuantumNous/new-api/deploy/lab/governorlab"
)
@@
- encoder := json.NewEncoder(os.Stdout)
- encoder.SetIndent("", " ")
- if err := encoder.Encode(result); err != nil {
+ payload, err := common.Marshal(result)
+ if err != nil {
+ log.Fatal(err)
+ }
+ if _, err := os.Stdout.Write(append(payload, '\n')); err != nil {
log.Fatal(err)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/lab/cmd/governor-lab-bootstrap/main.go` at line 5, The file imports
and calls the stdlib package encoding/json directly (e.g.
json.Marshal/json.Unmarshal) which violates the repo rule; replace that import
and all direct uses with the project JSON wrapper implemented in common/json.go
(use the wrapper's Marshal/Unmarshal functions exposed by that package),
updating import and any calls in main.go (including the usage around the code
previously using encoding/json) so all serialization/deserialization goes
through the common/json.go helpers.
Comment on lines
+155
to
+164
| lines := []string{ | ||
| "GOVERNOR_LAB_BASE_URL=" + quoteEnvValue(result.BaseURL), | ||
| "GOVERNOR_LAB_USER_ID=" + strconv.Itoa(result.UserID), | ||
| "GOVERNOR_LAB_CHANNEL_ID=" + strconv.Itoa(result.ChannelID), | ||
| "GOVERNOR_LAB_TOKEN_ID=" + strconv.Itoa(result.TokenID), | ||
| "GOVERNOR_LAB_API_KEY=" + quoteEnvValue(result.APIKey), | ||
| "GOVERNOR_LAB_MODEL=" + quoteEnvValue(result.Model), | ||
| } | ||
| content := strings.Join(lines, "\n") + "\n" | ||
| return os.WriteFile(path, []byte(content), 0o644) |
Contributor
There was a problem hiding this comment.
Write the generated env file with secret-safe permissions.
This file contains GOVERNOR_LAB_API_KEY, but it is created with mode 0644, so any local user on the host can read it. That should be 0600.
Suggested fix
- return os.WriteFile(path, []byte(content), 0o644)
+ return os.WriteFile(path, []byte(content), 0o600)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| lines := []string{ | |
| "GOVERNOR_LAB_BASE_URL=" + quoteEnvValue(result.BaseURL), | |
| "GOVERNOR_LAB_USER_ID=" + strconv.Itoa(result.UserID), | |
| "GOVERNOR_LAB_CHANNEL_ID=" + strconv.Itoa(result.ChannelID), | |
| "GOVERNOR_LAB_TOKEN_ID=" + strconv.Itoa(result.TokenID), | |
| "GOVERNOR_LAB_API_KEY=" + quoteEnvValue(result.APIKey), | |
| "GOVERNOR_LAB_MODEL=" + quoteEnvValue(result.Model), | |
| } | |
| content := strings.Join(lines, "\n") + "\n" | |
| return os.WriteFile(path, []byte(content), 0o644) | |
| lines := []string{ | |
| "GOVERNOR_LAB_BASE_URL=" + quoteEnvValue(result.BaseURL), | |
| "GOVERNOR_LAB_USER_ID=" + strconv.Itoa(result.UserID), | |
| "GOVERNOR_LAB_CHANNEL_ID=" + strconv.Itoa(result.ChannelID), | |
| "GOVERNOR_LAB_TOKEN_ID=" + strconv.Itoa(result.TokenID), | |
| "GOVERNOR_LAB_API_KEY=" + quoteEnvValue(result.APIKey), | |
| "GOVERNOR_LAB_MODEL=" + quoteEnvValue(result.Model), | |
| } | |
| content := strings.Join(lines, "\n") + "\n" | |
| return os.WriteFile(path, []byte(content), 0o600) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/lab/governorlab/bootstrap.go` around lines 155 - 164, The env file is
written with world-readable permissions; update the call that writes the file
(the os.WriteFile invocation in bootstrap.go that writes content created from
the lines slice including GOVERNOR_LAB_API_KEY) to use secret-safe permissions
(0600 / 0o600) instead of 0644 so only the owner can read/write the file; locate
the code that constructs lines (uses quoteEnvValue and result) and change the
file mode argument passed to os.WriteFile(path, []byte(content), ...) to 0o600.
Comment on lines
+391
to
+435
| func (c *Client) doJSON(ctx context.Context, method, path string, userID int, payload any, out any) error { | ||
| var body io.Reader | ||
| if payload != nil { | ||
| jsonBody, err := json.Marshal(payload) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| body = bytes.NewReader(jsonBody) | ||
| } | ||
|
|
||
| req, err := http.NewRequestWithContext(ctx, method, c.baseURL+path, body) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| req.Header.Set("Content-Type", "application/json") | ||
| if userID > 0 { | ||
| req.Header.Set("New-Api-User", strconv.Itoa(userID)) | ||
| } | ||
|
|
||
| resp, err := c.httpClient.Do(req) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode < 200 || resp.StatusCode >= 300 { | ||
| rawBody, _ := io.ReadAll(resp.Body) | ||
| return fmt.Errorf("request %s %s returned status %d: %s", method, path, resp.StatusCode, strings.TrimSpace(string(rawBody))) | ||
| } | ||
|
|
||
| var envelope apiEnvelope | ||
| if err := json.NewDecoder(resp.Body).Decode(&envelope); err != nil { | ||
| return err | ||
| } | ||
| if !envelope.Success { | ||
| if envelope.Message == "" { | ||
| envelope.Message = "request failed" | ||
| } | ||
| return fmt.Errorf("%s %s failed: %s", method, path, envelope.Message) | ||
| } | ||
| if out == nil || len(envelope.Data) == 0 || bytes.Equal(bytes.TrimSpace(envelope.Data), []byte("null")) { | ||
| return nil | ||
| } | ||
| return json.Unmarshal(envelope.Data, out) | ||
| } |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for common/json.go
fd -type f -name "json.go" | grep -i common | head -20Repository: QuantumNous/new-api
Length of output: 233
🏁 Script executed:
#!/bin/bash
# Also check the overall structure
git ls-files | grep -E "common.*json|json.*common" | head -20Repository: QuantumNous/new-api
Length of output: 77
🏁 Script executed:
#!/bin/bash
# Verify the imports in the bootstrap.go file
head -50 deploy/lab/governorlab/bootstrap.go | grep -E "^import|encoding/json"Repository: QuantumNous/new-api
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Inspect common/json.go to see available wrapper functions
cat common/json.goRepository: QuantumNous/new-api
Length of output: 817
🏁 Script executed:
#!/bin/bash
# Check the context of bootstrap.go - what is it for? Look at comments and package declaration
head -100 deploy/lab/governorlab/bootstrap.go | tail -60Repository: QuantumNous/new-api
Length of output: 1204
🏁 Script executed:
#!/bin/bash
# Verify the exact imports in bootstrap.go to confirm encoding/json is used directly
rg -A 5 "^import" deploy/lab/governorlab/bootstrap.go | head -20Repository: QuantumNous/new-api
Length of output: 121
Replace direct encoding/json calls with repository wrapper functions.
The doJSON method uses encoding/json directly in three places: json.Marshal(), json.NewDecoder().Decode(), and json.Unmarshal(). Per project guidelines, all JSON operations in Go files must route through common/json.go wrapper functions. Replace with:
json.Marshal(payload)→common.Marshal(payload)json.NewDecoder(resp.Body).Decode(&envelope)→common.DecodeJson(resp.Body, &envelope)json.Unmarshal(envelope.Data, out)→common.Unmarshal(envelope.Data, out)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/lab/governorlab/bootstrap.go` around lines 391 - 435, In
Client.doJSON, replace direct encoding/json usage with the repo wrappers: use
common.Marshal(payload) instead of json.Marshal(payload) when building the
request body; use common.DecodeJson(resp.Body, &envelope) instead of
json.NewDecoder(resp.Body).Decode(&envelope) to parse the API envelope; and use
common.Unmarshal(envelope.Data, out) instead of json.Unmarshal(envelope.Data,
out) to unmarshal the envelope.Data into out — keep existing error handling, nil
checks for out/envelope.Data, and the apiEnvelope/envelope variable names
intact.
Comment on lines
+125
to
+136
| func writeMockStreamResponse(w http.ResponseWriter, modelName, responseText string) { | ||
| w.Header().Set("Content-Type", "text/event-stream; charset=utf-8") | ||
| w.Header().Set("Cache-Control", "no-cache") | ||
| w.Header().Set("Connection", "keep-alive") | ||
| w.WriteHeader(http.StatusOK) | ||
|
|
||
| flusher, ok := w.(http.Flusher) | ||
| if !ok { | ||
| writeMockJSON(w, http.StatusInternalServerError, map[string]any{ | ||
| "error": map[string]any{"message": "streaming not supported"}, | ||
| }) | ||
| return |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect header-write order and flusher check in stream response helper
rg -n -C4 'func writeMockStreamResponse|WriteHeader\(|http\.Flusher|writeMockJSON\(' deploy/lab/governorlab/mock.goRepository: QuantumNous/new-api
Length of output: 2456
Move header writes and status code after flusher capability check.
Line 129 writes the 200 OK status before verifying http.Flusher support on line 131. If the flusher check fails, the error handler on lines 133–136 calls writeMockJSON(), which attempts to write a 500 status—but since WriteHeader() was already called with 200 OK, the error status is silently ignored by the HTTP runtime, returning an inconsistent 200 OK response with error JSON.
Fix
func writeMockStreamResponse(w http.ResponseWriter, modelName, responseText string) {
- w.Header().Set("Content-Type", "text/event-stream; charset=utf-8")
- w.Header().Set("Cache-Control", "no-cache")
- w.Header().Set("Connection", "keep-alive")
- w.WriteHeader(http.StatusOK)
-
flusher, ok := w.(http.Flusher)
if !ok {
writeMockJSON(w, http.StatusInternalServerError, map[string]any{
"error": map[string]any{"message": "streaming not supported"},
})
return
}
+ w.Header().Set("Content-Type", "text/event-stream; charset=utf-8")
+ w.Header().Set("Cache-Control", "no-cache")
+ w.Header().Set("Connection", "keep-alive")
+ w.WriteHeader(http.StatusOK)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func writeMockStreamResponse(w http.ResponseWriter, modelName, responseText string) { | |
| w.Header().Set("Content-Type", "text/event-stream; charset=utf-8") | |
| w.Header().Set("Cache-Control", "no-cache") | |
| w.Header().Set("Connection", "keep-alive") | |
| w.WriteHeader(http.StatusOK) | |
| flusher, ok := w.(http.Flusher) | |
| if !ok { | |
| writeMockJSON(w, http.StatusInternalServerError, map[string]any{ | |
| "error": map[string]any{"message": "streaming not supported"}, | |
| }) | |
| return | |
| func writeMockStreamResponse(w http.ResponseWriter, modelName, responseText string) { | |
| flusher, ok := w.(http.Flusher) | |
| if !ok { | |
| writeMockJSON(w, http.StatusInternalServerError, map[string]any{ | |
| "error": map[string]any{"message": "streaming not supported"}, | |
| }) | |
| return | |
| } | |
| w.Header().Set("Content-Type", "text/event-stream; charset=utf-8") | |
| w.Header().Set("Cache-Control", "no-cache") | |
| w.Header().Set("Connection", "keep-alive") | |
| w.WriteHeader(http.StatusOK) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/lab/governorlab/mock.go` around lines 125 - 136, In
writeMockStreamResponse, check for http.Flusher support before sending any
headers or calling w.WriteHeader; if the type assertion to flusher fails, call
writeMockJSON with the 500 status and return without having written a 200. After
confirming flusher (flusher, ok := w.(http.Flusher)), then set the
Content-Type/Cache-Control/Connection headers and call
w.WriteHeader(http.StatusOK) before streaming; this ensures writeMockJSON can
send the correct error status when streaming is unsupported.
|
|
||
| deadline=$((SECONDS + START_TIMEOUT_SECONDS)) | ||
| while (( SECONDS < deadline )); do | ||
| if curl -fsS "http://127.0.0.1:${MOCK_PORT}/healthz" >/dev/null 2>&1; then |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n deploy/lab/start-wsl-mock-openai.shRepository: QuantumNous/new-api
Length of output: 2227
Add curl timeouts so startup deadline is deterministic.
At Line 35, curl has no per-request timeout, so a single hung connection can block for system default TCP timeout (20+ seconds), preventing the health check loop from completing within START_TIMEOUT_SECONDS. This causes unpredictable startup behavior and wastes the deadline window on a single slow probe.
Proposed fix
- if curl -fsS "http://127.0.0.1:${MOCK_PORT}/healthz" >/dev/null 2>&1; then
+ if curl --connect-timeout 1 --max-time 2 -fsS "http://127.0.0.1:${MOCK_PORT}/healthz" >/dev/null 2>&1; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if curl -fsS "http://127.0.0.1:${MOCK_PORT}/healthz" >/dev/null 2>&1; then | |
| if curl --connect-timeout 1 --max-time 2 -fsS "http://127.0.0.1:${MOCK_PORT}/healthz" >/dev/null 2>&1; then |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deploy/lab/start-wsl-mock-openai.sh` at line 35, The health-check curl call
that probes http://127.0.0.1:${MOCK_PORT}/healthz can hang indefinitely; update
the conditional that calls curl to include per-request timeouts (e.g.,
--connect-timeout and --max-time) so each probe fails fast and the overall loop
bounded by START_TIMEOUT_SECONDS is deterministic; modify the curl invocation in
the start-wsl-mock-openai.sh health check (the line using curl ...
"http://127.0.0.1:${MOCK_PORT}/healthz") to add appropriate timeout flags
(choose small sensible values such as a short connect timeout and a slightly
larger total max time) so a single slow connection cannot consume the startup
deadline.
Comment on lines
+276
to
+278
| if !common.MemoryCacheEnabled { | ||
| _ = channel.SaveChannelInfo() | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t swallow polling-index persistence failures.
When common.MemoryCacheEnabled is false, CommitSelectedKeyIndex() only advances polling state through SaveChannelInfo(). Ignoring that error means the request can proceed even though the next selection will reuse a stale MultiKeyPollingIndex, which breaks polling fairness and can repeatedly hit the same key.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@model/channel.go` around lines 276 - 278, CommitSelectedKeyIndex currently
calls channel.SaveChannelInfo() when !common.MemoryCacheEnabled but ignores its
error; instead, capture and handle the error from SaveChannelInfo() inside
CommitSelectedKeyIndex (or propagate it to the caller) so failures prevent
advancing the polling index and request processing; update
CommitSelectedKeyIndex to check the returned error from
channel.SaveChannelInfo(), return or log and return the error appropriately
(preserving existing error semantics), referencing the CommitSelectedKeyIndex
and SaveChannelInfo methods and the MultiKeyPollingIndex state so the caller
doesn't proceed with a stale index.
Comment on lines
+65
to
+77
| var storeFactory = func() Store { | ||
| if !common.RedisEnabled || common.RDB == nil { | ||
| return nil | ||
| } | ||
| return NewRedisStore(common.RDB) | ||
| } | ||
|
|
||
| func SetStoreFactoryForTest(factory func() Store) func() { | ||
| previous := storeFactory | ||
| storeFactory = factory | ||
| return func() { | ||
| storeFactory = previous | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify all reads/writes of storeFactory and whether they are synchronized.
rg -n -C3 --type=go '\bstoreFactory\b'Repository: QuantumNous/new-api
Length of output: 2292
Synchronize storeFactory access to prevent data races.
The global storeFactory variable is read by multiple goroutines in manager.go (lines 30, 165, 187, 263) while SetStoreFactoryForTest writes to it without synchronization. Use sync.RWMutex to protect all reads and writes:
var (
storeFactoryMu sync.RWMutex
storeFactory = func() Store { /* ... */ }
)
// In SetStoreFactoryForTest
storeFactoryMu.Lock()
defer storeFactoryMu.Unlock()
// ... mutations ...
// In manager.go call sites
storeFactoryMu.RLock()
store := storeFactory()
storeFactoryMu.RUnlock()Alternatively, use sync/atomic.Pointer for lock-free synchronization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/governor/config.go` around lines 65 - 77, The global storeFactory is
mutated by SetStoreFactoryForTest and read concurrently by manager.go, causing
data races; introduce a package-level sync.RWMutex (e.g., storeFactoryMu) to
protect access: acquire storeFactoryMu.Lock()/Unlock() around the write in
SetStoreFactoryForTest and use storeFactoryMu.RLock()/RUnlock() at every call
site that invokes storeFactory() in manager.go (and any other readers), or
alternatively replace storeFactory with a sync/atomic.Pointer to the function
for lock-free reads/writes; update references to use the chosen synchronization
primitive around storeFactory accesses.
Comment on lines
+40
to
+54
| orderedIndices, apiErr := channel.OrderedEnabledKeyIndices() | ||
| if apiErr != nil { | ||
| return "", 0, apiErr | ||
| } | ||
|
|
||
| applyKeyConcurrency := cfg.KeyMaxConcurrency > 0 && !isAsyncTaskSubmit(c) | ||
| for _, keyIndex := range orderedIndices { | ||
| if cooling, _, err := store.IsKeyCooling(ctx, channel.Id, keyIndex); err == nil && cooling { | ||
| continue | ||
| } | ||
|
|
||
| keyValue, keyErr := channel.KeyAt(keyIndex) | ||
| if keyErr != nil { | ||
| return "", 0, keyErr | ||
| } |
Contributor
There was a problem hiding this comment.
Preserve raw channel.Key semantics for non-multi channels.
This path now resolves keys through OrderedEnabledKeyIndices() + KeyAt() even when channel.ChannelInfo.IsMultiKey is false. That changes behavior from GetNextEnabledKey(), which returns channel.Key verbatim for single-key channels. Channels that intentionally store structured credentials in Key (for example the JSON-array form handled in GetKeys()) will start sending only one parsed element once governor is enabled.
Suggested fix
- orderedIndices, apiErr := channel.OrderedEnabledKeyIndices()
- if apiErr != nil {
- return "", 0, apiErr
- }
+ orderedIndices := []int{0}
+ if channel.ChannelInfo.IsMultiKey {
+ orderedIndices, apiErr := channel.OrderedEnabledKeyIndices()
+ if apiErr != nil {
+ return "", 0, apiErr
+ }
+ }
applyKeyConcurrency := cfg.KeyMaxConcurrency > 0 && !isAsyncTaskSubmit(c)
for _, keyIndex := range orderedIndices {
if cooling, _, err := store.IsKeyCooling(ctx, channel.Id, keyIndex); err == nil && cooling {
continue
}
- keyValue, keyErr := channel.KeyAt(keyIndex)
- if keyErr != nil {
- return "", 0, keyErr
- }
+ keyValue := channel.Key
+ if channel.ChannelInfo.IsMultiKey {
+ keyValue, keyErr := channel.KeyAt(keyIndex)
+ if keyErr != nil {
+ return "", 0, keyErr
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| orderedIndices, apiErr := channel.OrderedEnabledKeyIndices() | |
| if apiErr != nil { | |
| return "", 0, apiErr | |
| } | |
| applyKeyConcurrency := cfg.KeyMaxConcurrency > 0 && !isAsyncTaskSubmit(c) | |
| for _, keyIndex := range orderedIndices { | |
| if cooling, _, err := store.IsKeyCooling(ctx, channel.Id, keyIndex); err == nil && cooling { | |
| continue | |
| } | |
| keyValue, keyErr := channel.KeyAt(keyIndex) | |
| if keyErr != nil { | |
| return "", 0, keyErr | |
| } | |
| var apiErr error | |
| orderedIndices := []int{0} | |
| if channel.ChannelInfo.IsMultiKey { | |
| orderedIndices, apiErr = channel.OrderedEnabledKeyIndices() | |
| if apiErr != nil { | |
| return "", 0, apiErr | |
| } | |
| } | |
| applyKeyConcurrency := cfg.KeyMaxConcurrency > 0 && !isAsyncTaskSubmit(c) | |
| for _, keyIndex := range orderedIndices { | |
| if cooling, _, err := store.IsKeyCooling(ctx, channel.Id, keyIndex); err == nil && cooling { | |
| continue | |
| } | |
| keyValue := channel.Key | |
| if channel.ChannelInfo.IsMultiKey { | |
| var keyErr error | |
| keyValue, keyErr = channel.KeyAt(keyIndex) | |
| if keyErr != nil { | |
| return "", 0, keyErr | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/governor/manager.go` around lines 40 - 54, The code currently
iterates OrderedEnabledKeyIndices() and uses channel.KeyAt() for all channels,
which changes semantics for single-key channels; update manager.go to preserve
raw channel.Key for non-multi channels by checking
channel.ChannelInfo.IsMultiKey (or using GetNextEnabledKey()/channel.Key
directly) before using OrderedEnabledKeyIndices()/KeyAt(): if the channel is not
multi-key, fetch and return/submit the raw channel.Key (respecting cooling via
store.IsKeyCooling for the single key if needed) instead of parsing via KeyAt();
otherwise continue to use OrderedEnabledKeyIndices() and KeyAt() for multi-key
channels.
Comment on lines
+192
to
+195
| decision := ClassifyTaskError(attempt.Config, taskErr) | ||
| if decision.CoolChannel && decision.TTL > 0 { | ||
| _ = store.CoolChannel(requestContext(c), attempt.ChannelID, decision.TTL) | ||
| } |
Contributor
There was a problem hiding this comment.
Apply key-level cooling for task attempts too.
CompleteTaskAttemptFromContext() ignores decision.CoolKey, so a task-specific failure on one key can immediately be reselected on the next submission instead of cooling just that bad key.
Suggested fix
decision := ClassifyTaskError(attempt.Config, taskErr)
if decision.CoolChannel && decision.TTL > 0 {
_ = store.CoolChannel(requestContext(c), attempt.ChannelID, decision.TTL)
}
+ if decision.CoolKey && decision.TTL > 0 {
+ _ = store.CoolKey(requestContext(c), attempt.ChannelID, attempt.KeyIndex, decision.TTL)
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/governor/manager.go` around lines 192 - 195,
CompleteTaskAttemptFromContext currently only applies channel-level cooling; if
ClassifyTaskError sets decision.CoolKey we must also apply key-level cooling so
the specific bad key is suppressed. In CompleteTaskAttemptFromContext, after the
existing check that calls store.CoolChannel(requestContext(c),
attempt.ChannelID, decision.TTL), add a similar guarded call when
decision.CoolKey && decision.TTL > 0 to call the store's key-cooling API with
the current request context, attempt.ChannelID and attempt.Key (preserving the
channel cooling behavior).
Comment on lines
+259
to
+279
| func stopAndRelease(c *gin.Context, attempt *AttemptState) { | ||
| if attempt == nil { | ||
| return | ||
| } | ||
| store := storeFactory() | ||
| releasePreparedAttempt(requestContext(c), store, attempt) | ||
| } | ||
|
|
||
| func releasePreparedAttempt(ctx context.Context, store Store, attempt *AttemptState) { | ||
| if attempt == nil { | ||
| return | ||
| } | ||
| if attempt.StopHeartbeat != nil { | ||
| attempt.StopHeartbeat() | ||
| attempt.StopHeartbeat = nil | ||
| } | ||
| if !attempt.LeaseHeld || attempt.ReservationID == "" || store == nil { | ||
| return | ||
| } | ||
| _ = store.ReleaseKeyLease(ctx, attempt.ChannelID, attempt.KeyIndex, attempt.ReservationID) | ||
| attempt.LeaseHeld = false |
Contributor
There was a problem hiding this comment.
Use a cleanup context when releasing leases.
Lease release currently uses requestContext(c). If the request has already been canceled or timed out, ReleaseKeyLease() inherits that canceled context and the cleanup is skipped, leaving the reservation occupied until TTL expiry.
Suggested fix
func stopAndRelease(c *gin.Context, attempt *AttemptState) {
if attempt == nil {
return
}
store := storeFactory()
- releasePreparedAttempt(requestContext(c), store, attempt)
+ ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+ defer cancel()
+ releasePreparedAttempt(ctx, store, attempt)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/governor/manager.go` around lines 259 - 279, The release path
currently uses requestContext(c) which may be canceled; change stopAndRelease to
create and pass a dedicated non-cancelable cleanup context (e.g.
context.WithTimeout(context.Background(), 5*time.Second) with cancel deferred)
into releasePreparedAttempt so ReleaseKeyLease(ctx, ...) always runs for
AttemptState cleanup; alternatively, inside releasePreparedAttempt wrap the call
to store.ReleaseKeyLease in a short background timeout context to ensure the
lease release executes even if requestContext(c) is canceled. Reference:
stopAndRelease, releasePreparedAttempt, AttemptState.StopHeartbeat,
AttemptState.LeaseHeld, AttemptState.ReservationID, and store.ReleaseKeyLease.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
wsl -d Ubuntu-24.04 -- bash -lc "cd /mnt/c/Users/ADMIN/.config/superpowers/worktrees/new-api/codex-new-api-governor && go test ./... -count=1"wsl -d Ubuntu-24.04 -- bash -lc "cd /mnt/c/Users/ADMIN/.config/superpowers/worktrees/new-api/codex-new-api-governor/web && NODE_OPTIONS=--max-old-space-size=4096 ./node_modules/.bin/vite build"wsl -d Ubuntu-24.04 --cd /root/workspaces/new-api-governor-lab/deploy/lab ./verify-wsl-governor-e2e.shNotes
bun run buildin this worktree was PATH-sensitive forvite, so frontend verification used the direct binary withNODE_OPTIONS=--max-old-space-size=4096.Summary by CodeRabbit
Release Notes
New Features
Testing
Chores