Repository navigation
cp: fix(server): enable TCP keepalive on accepted server connections (3727) into r0.7.0 - #3944
Merged
Merged
Conversation
Contributor
Author
|
/ok to test f5092a4 |
ananthsub
approved these changes
Oct 5, 2026
ananthsub
force-pushed
the
cherry-pick-3727-r0.7.0
branch
from
October 5, 2026 01:48
f5092a4 to
2dba92d
Compare
github-actions
Bot
added
the
sla:review-overdue
## What
Enable TCP keepalive on every connection the Gym server's uvicorn
accepts. It's done through `KeepaliveHttpToolsProtocol`, a subclass of
uvicorn's httptools protocol, passed as
`http=partial(KeepaliveHttpToolsProtocol, keepalive=(idle, interval,
probes))`.
- **No new config keys:** it reuses the existing
`global_aiohttp_tcp_keepalive_{idle_seconds,interval_seconds,probes}`
settings (60 / 10 / 3), which already apply to outgoing aiohttp
connections. The socket-option code is shared with the client socket
factory (`_set_tcp_keepalive`).
- **httptools stays mandatory (#2869):** no h11 fallback. A missing or
incompatible wheel still fails before the server starts.
- **Multi-worker support:** the protocol is importable at module level
and pickles with the uvicorn config, as multi-worker uvicorn requires.
- **Non-TCP sockets** (Unix sockets) are skipped.
This is a **minimal alternative to #3698**. It drops the SSE heartbeat
and the retry-after-commit logic, so streaming error behaviour is
unchanged from `main`.
## Why
A model server answers only after generation finishes, so an agent's
connection can carry no bytes for many minutes.
- Between sandbox cell-3 and the CMH cluster, a stateful hop on the
corporate transit path drops flows idle for about 6–7 minutes.
- The reply is then silently lost, and the agent (OpenCode) waits on a
half-open connection until its 3-hour limit.
- This is what caused TB 2.1 scores on cell-3 to trail cell-2 by 3–4
points.
Kernel keepalive probes after 60 s of idle keep the flow alive, and let
the kernel clean up peers that are really gone. No issue is filed; the
investigation and the A/B runs were done directly on the affected cell.
## Validation
- `pytest tests/unit_tests/test_server_utils.py`: 79 passed.
- New tests cover: config wiring (defaults and overrides of the reused
keys), pickling of the protocol for multi-worker uvicorn, and
`connection_made` setting keepalive on a real TCP socket and skipping
Unix sockets.
- The TCP test fails when keepalive is removed from `connection_made`.
- `pre-commit run --files nemo_gym/server_utils.py
tests/unit_tests/test_server_utils.py`: all hooks pass.
- Full `pytest tests/unit_tests` on macOS: 47 failures in CLI,
benchmark, compose and provider tests, plus `telemetry/test_memory.py`.
**All of them fail identically on unmodified `main`** in the same
environment (local-environment issues); none are in touched code.
## Rollout evidence
Terminal-Bench 2.1 (OpenCode, 89 tasks × 8),
`nano-3.5-sft-v24…step-6327`, on sandbox cell-3, vLLM 0.30.0 on CMH:
| Configuration | Runs | Reward | OpenCode calls hung until the 3h kill
|
|---|---|---|---|
| **Keepalive only** (#3698 with the SSE heartbeat disabled via config;
runtime-equivalent to this PR) | 3 | 0.397 / 0.400 / 0.417 (mean
**0.405**) | **0** |
| Keepalive + SSE heartbeat + retry (full #3698) | 6 | 0.383–0.421 (mean
0.406) | 0 |
| No fix (Gym `main`) | 5 | 0.30–0.33 at step 3996, 0.372 at step 6327 |
158–185 per run |
| Cell-2 reference (path not affected) | 6 | mean 0.396 | 0 |
"Hung" means the last assistant call was still silent ≥90 minutes when
the 3-hour kill hit. The keepalive-only runs used #3698's server-side
keepalive (same 60/10/3 values) with
`model_server_sse_heartbeat_grace_seconds=1000000`, so no heartbeat or
post-commit retry ever ran; that's the same runtime behaviour as this
PR. I haven't yet run an e2e rollout from this exact branch.
## Compatibility
No config migration. Existing `global_aiohttp_tcp_keepalive_*` overrides
now also tune the server side; the defaults are unchanged. No
benchmark-result impact on unaffected networks.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
---------
Signed-off-by: Hemil Desai <hemildesai@users.noreply.github.com>
Co-authored-by: Hemil Desai <hemildesai@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Signed-off-by: NeMo Bot <nemo-bot@nvidia.com>
ananthsub
force-pushed
the
cherry-pick-3727-r0.7.0
branch
from
October 6, 2026 20:33
2dba92d to
0f5a5bc
Compare
Contributor
|
/ok to test 0f5a5bc |
Contributor
|
/ok to test 0f5a5bc |
@ananthsub, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/2/ |
Contributor
|
/ok to test ade9d33 |
This branch was previously deployed
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
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.
beep boop [🤖]: Hi @hemildesai 👋,