router: skip preflight dial when no retries are configured by manaspros · Pull Request #490 · volcano-sh/agentcube

manaspros · 2026-10-27T05:14:27Z

What type of PR is this?

/kind enhancement

What this PR does / why we need it:

waitForUpstreamReachable dials the sandbox, closes the connection, and then the proxy connects again. cmd/router does not set InitialConnectRetryCount, so it is 0 in the real binary and the preflight has one attempt, which the proxy repeats anyway.

This PR:

returns from waitForUpstreamReachable early when InitialConnectRetryCount is 0;
gives the router's http.Transport a DialContext with Timeout: config.InitialConnectRetryInterval (200 ms by default). That is the dial timeout the preflight used, so an unreachable sandbox still fails after 200 ms and not after the OS connect timeout.

With retries configured (> 0) nothing changes.

The error a client sees is the same. I checked: upstreamUnavailableResponse maps both the preflight error and the proxy error, and a refused connection still returns 502. I added a test for that. The test also passes on main, which is the point.

Two commits, benchmark first. The benchmark is the same file as in the JWT PR (identical content), so whichever merges first, the other should rebase cleanly.

go test ./pkg/router -run xxx -bench BenchmarkInvoke -benchmem -benchtime=1000x -count=10
benchstat before.txt after.txt

goos: linux
goarch: amd64
pkg: github.com/volcano-sh/agentcube/pkg/router
cpu: AMD Ryzen 9 9900X 12-Core Processor            
                          │ D_before.txt │             D_after.txt             │
                          │    sec/op    │   sec/op     vs base                │
Invoke/SandboxClaim-24       607.4µ ± 1%   579.9µ ± 0%   -4.53% (p=0.000 n=10)
Invoke/CodeInterpreter-24    48.10µ ± 4%   29.35µ ± 6%  -38.98% (p=0.000 n=10)
geomean                      170.9µ        130.5µ       -23.68%

                          │ D_before.txt │             D_after.txt             │
                          │     B/op     │     B/op      vs base               │
Invoke/SandboxClaim-24      55.64Ki ± 0%   52.15Ki ± 0%  -6.28% (p=0.000 n=10)
Invoke/CodeInterpreter-24   51.00Ki ± 0%   47.39Ki ± 0%  -7.08% (p=0.000 n=10)
geomean                     53.27Ki        49.72Ki       -6.68%

                          │ D_before.txt │            D_after.txt             │
                          │  allocs/op   │ allocs/op   vs base                │
Invoke/SandboxClaim-24        241.0 ± 0%   198.0 ± 1%  -17.84% (p=0.000 n=10)
Invoke/CodeInterpreter-24     194.0 ± 0%   151.0 ± 0%  -22.16% (p=0.000 n=10)
geomean                       216.2        172.9       -20.03%

CodeInterpreter does not sign a JWT, so it shows the preflight saving alone: 48.10µ to 29.35µ, allocs 194 to 151. SandboxClaim is dominated by JWT signing (see my other PR), so the same saving is only 4.53 percent there. The upstream is on loopback, so in a cluster the saving per request is larger by one round trip.

Which issue(s) this PR fixes:
Fixes #489

Special notes for your reviewer:

What I did not test: a real cluster, a blackholed pod IP (the 200 ms dial timeout on the transport is there for this, I tested only the refused case), and a router started with retries configured. The existing TestWaitForUpstreamReachable_RetriesUntilReady covers the retry path and still passes.

I am not sure about one thing and would like a maintainer view. After this change a router that has no retries relies on the proxy for the first connect. #256 was about a race right after sandbox creation. The workload manager already probes the entrypoints before it returns the sandbox (waitForSandboxEntryPointsReady), and the retry knob is still there for anyone who sets it. If you want the preflight kept for newly created sessions, that is an easy change.

New tests: preflight does not accept a connection with 0 retries, and a refused upstream still returns 502 through the router. go test -race ./pkg/... passes. Lint: golangci-lint v2.15 with an equivalent config (the Makefile pins v1.64.1 but the config is v2), no new findings in non-test code.

AI use: I used Claude Code to find this and to draft the code, benchmark and tests. I read and ran all of it and I can explain each line.

Does this PR introduce a user-facing change?:

NONE

Benchmark handleInvoke end to end (session lookup, preflight dial, JWT signing, reverse proxy) against an in-process upstream, for the SandboxClaim kind that signs a JWT and the CodeInterpreter kind that does not. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: manaspros <manasnandchoudhary@gmail.com>

waitForUpstreamReachable opened and closed a TCP connection to the sandbox on every request, then the proxy connected again. cmd/router never sets InitialConnectRetryCount, so the preflight had a single attempt and could not wait for anything. Return early when the retry count is 0 and give the proxy transport the same dial timeout the preflight used, so an unreachable sandbox still fails after 200ms. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com> Signed-off-by: manaspros <manasnandchoudhary@gmail.com>

volcano-sh-bot · 2026-10-27T05:14:33Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign yaozengzeng for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

pkg/router/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copilot

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota.

codecov-commenter · 2026-10-27T05:20:27Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.46%. Comparing base (f521ae9) to head (e417aad).
⚠️ Report is 15 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #490      +/-   ##
==========================================
+ Coverage   60.45%   61.46%   +1.00%     
==========================================
  Files          35       35              
  Lines        3616     3643      +27     
==========================================
+ Hits         2186     2239      +53     
+ Misses       1211     1184      -27     
- Partials      219      220       +1

Flag	Coverage Δ
unittests	`61.46% <100.00%> (+1.00%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

zhzhuang-zju · 2026-10-27T08:11:14Z

+	// Without retries the preflight is one more connect to the sandbox that
+	// the proxy repeats right after. The proxy transport has the same dial
+	// timeout (see NewServer), so it reports the same errors.
+	if retryCount == 0 {


InitialConnectRetryCount is not exposed as a Router CLI flag, so deployed routers always use the default value of 0. Should we expose it as a flag if we intend to retain the retry path?

Also, InitialConnectRetryCount suggests the number of retries after an initial connection attempt. Using 0 to skip the initial attempt seems inconsistent with that meaning. WDYT?

manaspros and others added 2 commits October 27, 2026 10:43

Copilot AI balanced review requested due to automatic review settings October 27, 2026 05:14

volcano-sh-bot added the kind/enhancement New feature or request label Oct 27, 2026

volcano-sh-bot requested review from YaoZengzeng and acsoto October 27, 2026 05:14

Copilot AI reviewed Oct 27, 2026

volcano-sh-bot added the size/L label Oct 27, 2026

zhzhuang-zju reviewed Oct 27, 2026

View reviewed changes

Navigation Menu

Repository navigation

router: skip preflight dial when no retries are configured - #490

router: skip preflight dial when no retries are configured#490
manaspros wants to merge 2 commits into
volcano-sh:mainfrom
manaspros:perf/router-skip-preflight

manaspros commented Oct 27, 2026

Uh oh!

volcano-sh-bot commented Oct 27, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

codecov-commenter commented Oct 27, 2026

Uh oh!

zhzhuang-zju Oct 27, 2026

Uh oh!

zhzhuang-zju Oct 27, 2026

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

manaspros commented Oct 27, 2026

Uh oh!

volcano-sh-bot commented Oct 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Oct 27, 2026

Codecov Report

Uh oh!

zhzhuang-zju Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

zhzhuang-zju Oct 27, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants