fix(sdk): do not send a non-positive Connect-Timeout-Ms for a command - #1485
Conversation
![cubesandboxbot[bot]](https://avatars.githubusercontent.com/in/3653584?s=60&v=4){kind=small}
| # Both non-positive values arrive in ordinary use: 0 is how the e2b SDK | ||
| # spells "no deadline", and NEVER_TIMEOUT is how this one does. _pty.py | ||
| # and the Go SDK already guard this way. | ||
| if timeout is not None and timeout > 0: |
There was a problem hiding this comment.
The header is now correctly omitted for 0/NEVER_TIMEOUT, but this guard alone doesn't deliver the "no deadline" behavior the PR promises. _run_with_connect_fallback still passes the raw value through to the HTTP client: client.stream("POST", ..., timeout=timeout). For httpx, only None disables timeouts; 0 and -1 become immediate connect/read/write/pool timeouts on a real transport (httpcore applies them as deadline timers). So with a real connection, run(timeout=0) and run(timeout=NEVER_TIMEOUT) will still fail client-side (immediate connect/read timeout — or a ValueError on httpx versions that reject negative timeouts) instead of executing the command.
The new tests don't catch this because httpx.MockTransport bypasses httpcore's timeout machinery entirely — the handler returns a fully-buffered response synchronously, so no timeout is ever enforced. Suggest normalizing before the client call, e.g. client_timeout = timeout if (timeout is not None and timeout > 0) else None, and consider a real-transport test for these two values.
| // Both non-positive values arrive in ordinary use: `0` is how the e2b SDK | ||
| // spells "no deadline", and NEVER_TIMEOUT is how this one does. `pty.ts` | ||
| // and the Go SDK already guard this way. | ||
| if (idleMs !== undefined && idleMs > 0) { |
There was a problem hiding this comment.
Edge case in the same bug family: the guard is > 0 on the input, but the wire value is produced by truncation, so a positive-but-sub-millisecond input still emits the exact header this PR removes. timeoutMs: 0.5 passes 0.5 > 0 and then String(Math.trunc(0.5)) = "0". The Python twin has the same hole (timeout: 0.0005 → int(0.0005 * 1000) = 0). Unrealistic in practice, but closing it is a one-character change: guard on the truncated value (Math.trunc(idleMs) > 0).
Review of #1485 — fix(sdk): do not send a non-positive Connect-Timeout-Ms for a commandAI-generated review. Overall. The fix is correct for the code it touches and aligns Node/Python with the convention already present in the rest of the codebase: Go Findings.
|
9c4837e to
c411895
Compare
| # Both non-positive values arrive in ordinary use: 0 is how the e2b SDK | ||
| # spells "no deadline", and NEVER_TIMEOUT is how this one does. _pty.py | ||
| # and the Go SDK already guard this way. | ||
| if timeout is not None and timeout > 0: |
There was a problem hiding this comment.
The header guard is correct, but the same non-positive timeout is still forwarded to httpx as the client-side timeout a few lines below (self._sandbox._client.stream("POST", url, ..., timeout=timeout) in _run_with_connect_fallback). timeout=0 becomes httpx.Timeout(0) (immediate/non-blocking timeouts), and timeout=NEVER_TIMEOUT (-1) is an invalid negative socket timeout. So in real (non-mocked) usage this fallback path still won't get "no deadline" semantics for 0/NEVER_TIMEOUT — it will fail fast with a different, confusing transport error rather than hang.
The new tests use httpx.MockTransport, which never enforces client-side timeouts, so they can't detect this. Consider passing timeout=None to client.stream(...) when the value is non-positive (the header is already omitted in that case), so the command actually runs without a client-side deadline.
| // spells "no deadline", and NEVER_TIMEOUT is how this one does. `pty.ts` | ||
| // and the Go SDK already guard this way. | ||
| if (idleMs !== undefined && idleMs > 0) { | ||
| headers["Connect-Timeout-Ms"] = String(Math.trunc(idleMs)); |
There was a problem hiding this comment.
Minor: the guard checks the pre-truncation value, but the header is built from the truncated one. A positive-but-sub-millisecond value (e.g. timeoutMs: 0.5) passes idleMs > 0 yet Math.trunc(0.5) === 0, sending Connect-Timeout-Ms: 0 — exactly the "already-passed deadline" this PR removes. The Python side has the same gap (int(timeout * 1000) is 0 for 0 < timeout < 0.001). Guarding on the truncated value (Math.trunc(idleMs) > 0) would close the hole completely. Unlikely in practice, so flagging as low severity.
There was a problem hiding this comment.
@HuChundong Hi, the Node.js changes LGTM. However, the Python fix appears incomplete: non-positive timeouts are no longer sent to envd, but they are still passed to httpx. timeout=0 fails immediately, while timeout=-1 raises a ValueError.
I think non-positive values should be normalized to None, for example:
effective_timeout = timeout if timeout is not None and timeout > 0 else NoneThen effective_timeout should be passed to httpx instead of the original timeout.
There was a problem hiding this comment.
Thanks — you're right, and thanks for catching it. Fixed in ca207bc, using the variable name you suggested.
effective_timeout is now computed once and both readers of it take the normalised value: the header is omitted, and httpx is given None instead of the raw 0/-1. Without this the fix only moved the failure — from a hang to an immediate socket timeout (or a ValueError on httpx versions that reject a negative) — so neither value actually meant "no deadline" on that path.
The tests are updated too, because the existing ones could not have caught it: httpx.MockTransport answers synchronously and enforces no client-side timeout, so the value passed has no observable effect. They now record the timeout the client was handed (_recording_client) and assert None for 0/NEVER_TIMEOUT and 30 for a positive one. Reverting the one-line source change turns both parametrized cases red, so they hold the behaviour rather than describing it.
Two places I deliberately did not touch:
_pty.py— its httpx calls take a separaterequest_timeout, so the raw value never reaches a client there._run_with_e2b_connect—0is the e2b client's own spelling of "no deadline", so forwarding it is correct on that path.
On the bot's other note (a positive timeout below one millisecond truncating to Connect-Timeout-Ms: 0): I looked for a way to reach it and could not find one, so I have left it out of this PR rather than widen it. Nothing in the SDK computes a timeout and passes it down — commands.run takes the caller's value directly, and the only deadline = monotonic() + timeout in the tree is pause(wait=True)'s polling loop, which never feeds a command. The parameter's unit also excludes it (timeoutMs is documented in milliseconds; the Python one is seconds), and where this SDK does reject absurd timeouts — -2, NaN, Infinity on setTimeout — it rejects them at the entry point before any request, which seems the right layer for that class of value rather than the header builder. Happy to send it as a separate PR if you think it is worth closing anyway.
Local runs: sdk/python 257 passed / 4 skipped, sdk/node 216 passed / 2 skipped (untouched, as a regression baseline). The pre-existing ruff and prettier complaints on these files are the same on the base commit, so I have not reformatted them here.
There was a problem hiding this comment.
Two corrections to my previous comment, and an unrelated finding on the e2b path.
The lockfile is gone. sdk/python/uv.lock was staged by accident (a git add -A in my working tree) and was never meant to be part of this PR. Force-pushed without it; the PR is four files again, two of them from the first commit.
On the e2b path, I was half right and should have checked before saying it. I said 0 is that client's own spelling of "no deadline" and left the path alone. That much holds, but not for the reason I implied — it holds by falsiness, not by intent: connectrpc's sync client does timeout_ms=timeout_ms or self._timeout_ms before if timeout_ms is not None: headers["connect-timeout-ms"] = str(timeout_ms), so 0 falls through to the client default and no header is sent.
NEVER_TIMEOUT does not. -1 is truthy, so it reaches the header as connect-timeout-ms: -1, which is the same class of value this PR exists to stop sending. So the bot's note is right about that half.
I have not fixed it here, because of a second thing I ran into while checking, which looks more significant than the timeout question:
_run_with_e2b_connect does not match the current e2b signature. It calls
rpc.start(request, headers=..., timeout=timeout, request_timeout=...)but in e2b 2.45.1 (today's release) ProcessClient.start is start(self, request, *, headers=None, timeout_ms=None). There is no timeout and no request_timeout, so that call raises TypeError — and run() only catches ImportError, so it does not fall back; it propagates. e2b is not a declared dependency of this SDK (not in dependencies, not in the dev extra), so the path is exercised only on a machine that happens to have it installed, which is presumably why CI does not see this.
If that reading is right, then fixing the -1 header on that path means first deciding what the call should be — and whether the path should keep a timeout parameter at all, given the units differ (timeout in seconds here, timeout_ms there). That is a bigger change than this PR, and it is your call rather than mine. Happy to open a separate issue with the above, or a PR if you tell me which shape you want.
For this PR the scope is unchanged: the fallback path, which is the one that actually runs by default.
| return self._run_with_e2b_connect( | ||
| cmd, | ||
| timeout=timeout, | ||
| timeout=effective_timeout, |
There was a problem hiding this comment.
effective_timeout is not defined in this scope — it is only assigned inside _run_with_connect_fallback() further down. It is referenced here in run(), again in the except ImportError: branch (line ~75), and in _run_with_e2b_connect() (line ~114). Python resolves the name at call time in the local scope, so every sb.commands.run(...) raises NameError: name 'effective_timeout' is not defined before either dispatch path is reached — including the two new tests in this PR, which would error rather than pass. The reported "python 257 passed" run cannot match this diff.
Fix: compute it once in run() before the try:
effective_timeout = timeout if timeout is not None and timeout > 0 else Noneand pass effective_timeout to both helpers. The duplicate assignment inside _run_with_connect_fallback() then becomes redundant-but-idempotent (or can be removed).
ca207bc to
af1d18e
Compare
| # Both non-positive values arrive in ordinary use: 0 is how the e2b SDK | ||
| # spells "no deadline", and NEVER_TIMEOUT is how this one does. _pty.py | ||
| # and the Go SDK already guard this way. | ||
| effective_timeout = timeout if timeout is not None and timeout > 0 else None |
There was a problem hiding this comment.
This normalisation is correct for the fallback path, but the e2b path is not fixed: run() forwards the same timeout to _run_with_e2b_connect, which still passes the raw value to rpc.start(..., timeout=timeout, ...) at line 114. When the optional e2b package is installed, timeout=0 (e2b's spelling of "no deadline") or NEVER_TIMEOUT (-1) is still forwarded, and connect-python turns it into an already-expired / outright-rejected deadline — so the request fails immediately instead of running with no deadline, which is exactly the behavior this PR removes. The two Python code paths now disagree, and the new tests only cover this one (they force it with patch.object(Commands, "_run_with_e2b_connect", side_effect=ImportError)). Suggest applying the same normalisation in _run_with_e2b_connect too.
| @@ -0,0 +1,1015 @@ | |||
| version = 1 | |||
There was a problem hiding this comment.
This 1015-line lockfile is not mentioned in the PR body, and it isn't consumed by CI: sdk-test-check.yml and tests/unittest/run_sdk_test.sh both install via pip install -e '.[dev]' (pip cache keyed to pyproject.toml), and .gitignore deliberately doesn't exclude it, so it was committed on purpose. For a published library whose dev extras are un-pinned, a uv-specific lockfile is a tooling change that belongs in its own PR — suggest dropping it from this bugfix.
af1d18e to
f27bab7
Compare
| # Both non-positive values arrive in ordinary use: 0 is how the e2b SDK | ||
| # spells "no deadline", and NEVER_TIMEOUT is how this one does. _pty.py | ||
| # and the Go SDK already guard this way. | ||
| effective_timeout = timeout if timeout is not None and timeout > 0 else None |
There was a problem hiding this comment.
Question — does the fix also cover the primary e2b path?
Commands.run() tries _run_with_e2b_connect first and only reaches this fallback on ImportError. That primary path still forwards the raw timeout — including 0 and NEVER_TIMEOUT (-1) — to rpc.start(..., timeout=timeout, request_timeout=...). If e2b's generated client maps that timeout onto a Connect-Timeout-Ms header (the usual Connect-protocol deadline mechanism), the same "deadline already passed → hang" bug would persist for anyone who has the optional e2b package installed.
Since e2b isn't declared as a dependency in pyproject.toml, a bare cubesandbox install always uses the fallback, so the default install is fixed. But it would be good to confirm the e2b path handles non-positive timeouts too — and if it does, a one-line comment noting why would prevent a future regression here.
`commands.run(timeout=...)` reaches two readers that spell "no deadline" differently, and a non-positive value was mishandled by both. envd reads `Connect-Timeout-Ms` as a hard wall-clock deadline, so a zero or negative one has already passed: the request is never answered and the caller waits until its HTTP client gives up on headers, then reports a transport error that says nothing about the cause. httpx reads the same number as a client-side deadline, where only `None` means "no deadline" -- `0` makes every socket operation time out at once, and `-1` is rejected outright. Both non-positive values arrive in ordinary use: `0` is how the e2b SDK spells "no deadline", and `NEVER_TIMEOUT` is how this one does. `pty.ts`, `_pty.py` and the Go `setConnectTimeout` helper already guard the header this way; the command paths did not. The value is normalised once, into `effective_timeout`, and both readers take the normalised one: the header is omitted and httpx is given `None`. Not touched: `_pty.py`, whose httpx calls take a separate `request_timeout`, and `_run_with_e2b_connect`, where `0` is that client's own spelling of "no deadline" -- see the PR discussion for what `NEVER_TIMEOUT` does there and why it is not in scope here. The tests record the timeout the client was handed rather than its effect: `httpx.MockTransport` answers synchronously and enforces no timeout, so a forwarded `0` has nothing to observe. Reverting either half of the fix turns them red. Signed-off-by: carmake <gycm520@gmail.com>
f27bab7 to
abb3d10
Compare
| // Both non-positive values arrive in ordinary use: `0` is how the e2b SDK | ||
| // spells "no deadline", and NEVER_TIMEOUT is how this one does. `pty.ts` | ||
| // and the Go SDK already guard this way. | ||
| if (idleMs !== undefined && idleMs > 0) { |
There was a problem hiding this comment.
Truncation edge: the guard checks the raw idleMs, but the header is Math.trunc(idleMs). A positive idleMs in (0, 1) ms (e.g. timeoutMs: 0.5) passes idleMs > 0 and still sends Connect-Timeout-Ms: 0 — the exact value this fix is meant to suppress, which envd treats as an already-passed deadline. Guard on the truncated value instead: if (idleMs !== undefined && Math.trunc(idleMs) > 0).
| if effective_timeout is not None: | ||
| headers["Connect-Timeout-Ms"] = str(int(effective_timeout * 1000)) | ||
| access_token = self._sandbox._data.get("envdAccessToken") | ||
| if access_token: |
There was a problem hiding this comment.
Same truncation edge on the Python side: int(effective_timeout * 1000) truncates, so a positive timeout in (0, 0.001) seconds (e.g. timeout=0.0005) passes the > 0 guard yet emits Connect-Timeout-Ms: 0, reintroducing the non-positive deadline. Consider computing the milliseconds first and only setting the header when that value is > 0.
|
Thanks for your contribution, LGTM |
3 participants
Fixes #1484.
What
commands.runforwardedtimeoutMsto envd asConnect-Timeout-Mswithout checking that it waspositive. envd reads that header as a hard wall-clock deadline, so a zero or negative one has
already passed: the request is never answered, and the caller waits until its HTTP client gives up
on headers and reports a transport error that says nothing about the cause.
Both non-positive values arrive in ordinary use:
0is how thee2bSDK spells "no deadline", so it comes with exactly the port the README'sdrop-in compatibility invites.
NEVER_TIMEOUTis how this SDK spells it, and its value is-1— a constant meaning "doesnot time out" whose effect was that nothing ever returned.
Why this is a slip rather than a design choice
The same guard is already correct everywhere else it appears:
commandsptyif (idleMs !== undefined)← fixed hereif (options.timeoutMs !== undefined && options.timeoutMs > 0)if timeout is not None:← fixed hereif timeout is not None and timeout > 0:if timeout <= 0 { return }What is not changed
Sandbox.create's owntimeoutfield.0andNEVER_TIMEOUTare meaningful to CubeAPI there,and
test_create_sends_explicit_zero_timeout/test_create_sends_never_timeoutstill passunchanged.
Tests
Node and Python both gain two cases: no
Connect-Timeout-Msheader for0orNEVER_TIMEOUT, anda header that is still sent for a positive value. Both new cases fail against the previous
behaviour — reverting the one-line guard turns them red and leaves the rest green.
Measured
Against a running sandbox, before the fix: