Skip to content

fix(proxy): probe upstream liveness with TCP keepalive - #114

Open
cleverhu wants to merge 2 commits into
kvcache-ai:mainfrom
cleverhu:fix/proxy-tcp-keepalive
Open

fix(proxy): probe upstream liveness with TCP keepalive#114
cleverhu wants to merge 2 commits into
kvcache-ai:mainfrom
cleverhu:fix/proxy-tcp-keepalive

Conversation

@cleverhu

@cleverhu cleverhu commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

What

Enable TCP keepalive probes on the sandbox proxy's connector: 3 s idle before the first probe, 1 s between probes, 2 retries.

Depends on #109. This branch is stacked on it because both touch build_proxy_client(); the diff shown here against main therefore also contains #109's commit. Please merge #109 first — this then rebases to a single-commit, three-line change.

Why

Tearing down a VM never delivers FIN or RST to the host side of a proxied connection. The socket stays open from the host's point of view, so a request addressed to a sandbox that is already gone hangs until some higher-level timeout fires instead of failing immediately.

Keepalive probes are what detect that silence. With the values above the failure surfaces in roughly 5 s, which is a prompt error for a caller and still far above any plausible pause in normal proxied traffic.

Related issue

None. Small, self-contained reliability fix on an existing code path.

Scope and non-goals

Included: keepalive parameters on the proxy connector.

Excluded:

  • Retry or failover behavior when a probe does detect a dead peer — the request fails, as it should.
  • Connection pooling. pool_max_idle_per_host(0) already handles the separate problem of interaction IPs being reused across runtime generations.
  • Any other connector in the tree.

Design and behavior changes

Three constants next to the existing PROXY_CONNECT_TIMEOUT, applied via HttpConnector::set_keepalive{,_interval,_retries}.

Failure mode change: a request to a torn-down sandbox now returns a connection error after ~5 s instead of hanging. Callers that previously relied on a longer timeout to mask this will see the error sooner, which is the intent.

Compatibility and operations

  • Public API or generated protocol: N/A.
  • Configuration or defaults: no new config keys. The values are compile-time constants; if maintainers want them tunable I can move them into [orchestrator] or a proxy section.
  • Snapshot manifest, artifact layout, or storage format: N/A.
  • Upgrade and rollback: transparent both ways.
  • Host requirements, permissions, ports, or dependencies: TCP_KEEPIDLE/TCP_KEEPINTVL/TCP_KEEPCNT are Linux socket options and AgentENV is Linux-only.

Validation

  • make fmt
  • make clippy
  • make test-unit
  • Relevant Rust integration tests
  • make -C services test (required when services/ changes)
  • Generated clients/server regenerated with the documented make target
  • Documentation updated
  • Benchmarks or performance comparison completed

Commands and results:

$ cargo fmt --all -- --check
(clean)

$ cargo clippy --workspace --all-targets -- -D warnings
Finished `dev` profile

$ cargo check -p agentenv --all-targets
Finished `dev` profile

Skipped checks and reasons:

  • make test-unit not run. Observing this behavior in a test requires a peer that vanishes without sending FIN/RST, which is not reproducible with an in-process test server; it needs a real VM teardown or an injected netfilter drop.
  • No benchmark: this is a failure-mode fix, not a throughput or latency change on the healthy path.

Risks and reviewer notes

The risk worth reviewing is the aggressiveness of the values. A 3 s idle threshold with 1 s probes means an idle-but-alive sandbox connection sends a probe every second. That is negligible traffic on a host-local link, but if the proxy is ever used across a slower network the numbers deserve revisiting.

The other question is whether ~5 s to detect is the right target. It is well under typical client timeouts while leaving room for a transient stall.

Most important file: src/api/proxy.rs.

Checklist

  • The PR contains one coherent change and no unrelated formatting or refactoring.
  • New behavior is covered by tests, or I explained why testing is impractical.
  • Logs and examples contain no credentials, tokens, or private registry information.
  • I did not manually edit generated code without updating its source and regenerating it.

envd streams a command's lifecycle as a burst of tiny Connect-RPC
frames. With Nagle enabled, every frame after the first waits for the
peer's delayed ACK, which puts a ~40ms floor under each short-lived
command regardless of how long the command itself runs.

Every hop on that path exchanges small frames only, so the coalescing
Nagle trades latency for never materializes. Set TCP_NODELAY on the API
server's accepted connections, on the sandbox proxy's connector and on
both envd client connectors.
Tearing down a VM never delivers FIN/RST to the host side of a proxied
connection, so a request to a sandbox that is already gone hangs until
some higher-level timeout fires. Short keepalive probes turn that into a
prompt connection error instead.
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

OpenCodeReview: Review complete: 0 finding(s) across 3 selected item(s).

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant