Skip to content

Commit 1c3af28

Browse files
authored
fix: Kubernetes exec reported unknown status as success and leaked its socket (#74)
The last two class-level `# pragma: no cover` — `KubernetesPodSandbox` and `DaytonaSandbox` — and the fourth time removing one has surfaced a real bug. `_execute_api` defaulted a missing return code to 0. The read loop exits as soon as the output cap is reached, with the command still running and no code reported yet, so every truncated command came back as having passed. It now defaults to 1, which is the convention `LocalBackend` already used. `resp.close()` sat after the read loop inside the same `try`, so a connection dropping mid-command skipped it — one leaked websocket per failed command, for the life of the process. Moved to a `finally`, and opening the stream is now its own guarded step so a failure there is not confused with a read failure. 30 tests cover the pod-exec path, both readiness outcomes and the polling in between, liveness, config loading, the `mode="http"` listing failures and the `mode="api"` fallbacks to the shell, plus Daytona's readiness probe and failure handlers. No blanket pragma remains in the package.
1 parent d8ddebd commit 1c3af28

5 files changed

Lines changed: 541 additions & 10 deletions

File tree

CHANGELOG.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,11 +9,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
99

1010
### Added
1111

12+
- **`KubernetesPodSandbox` and `DaytonaSandbox` are covered.** The last two class-level `# pragma: no cover`; no blanket one remains in the package. 30 tests cover the pod-exec path, both readiness outcomes and the polling in between, liveness, config loading, the `mode="http"` listing failures and the `mode="api"` fallbacks to the shell — plus Daytona's readiness probe and failure handlers. The two exec bugs above are what the pass found.
1213
- **The console tools' own rendering is covered.** Every tool body sat behind a blanket `# pragma: no cover`, so how a listing, a glob, a grep or a background shell is actually *rendered for the model* was unmeasured — and both `hashline` variants, which register only under `edit_format="hashline"`, had no coverage at all. 29 tests now cover them: the empty and truncated forms of `ls`, `glob` and `grep`, `grep`'s count mode and its error passthrough, a hashline read and a full hashline edit round trip including a stale hash and a failing write-back, and the background tools with and without a sandbox that has a shell.
1314
- **`LocalBackend`'s failure and denial paths are covered.** 30 of its methods' error handlers sat behind a blanket `# pragma: no cover`, so in the backend most users touch, the code that turns a denied path or a filesystem error into a reportable result was never run by a test — including every `PermissionError` handler on its permission boundary. The pragmas are gone and 45 tests cover it: a path outside the allowed directories for every operation, an unreadable file and directory, an offset past the end, a read of a directory, `OSError` on read, write, edit and glob, and both grep implementations forced explicitly rather than left to whether `rg` happens to be installed — which is what made this file's coverage depend on the machine. The glob truncation above is what the pass found.
1415

1516
### Fixed
1617

18+
- **A Kubernetes exec no longer reports an unknown status as success.** `_execute_api` defaulted a missing return code to `0`, and the read loop exits as soon as the output cap is hit — with the command still running and no code yet. So **every truncated command was reported as having passed**. It now defaults to `1`, matching `LocalBackend`, which had the convention right.
19+
- **A Kubernetes exec no longer leaks a websocket on failure.** `resp.close()` sat after the read loop inside the same `try`, so a connection dropping mid-command skipped it entirely — one leaked socket per failed command, for the life of the process. Moved to a `finally`.
1720
- **A glob no longer silently returns a short answer.** `LocalBackend.glob_info` wrapped its whole walk in one `try`, so a single entry that could not be stat'd — deleted between the glob and the stat, or in a directory the process cannot read — aborted the loop and returned whatever had been collected. Measured on four matching files with one bad entry: **one** came back. The model gets an incomplete answer with nothing to indicate it, which is worse than a missing row and worse than an error. Now skipped per entry, matching `ls_info` and `grep_raw`, which both already carried on.
1821

1922
- **A listing no longer reports paths the sandbox cannot read back.** `ls_info` built each row's `path` from the *shell-quoted* directory, so listing `/my work` returned `'/my work'/notes.md` — a path that does not exist. A model handed that row and asking to read it got a failure it could not recover from, and the directory was effectively unreachable. Plain paths quote to themselves, which is why it went unnoticed. Affects every shell-derived sandbox: Docker, Daytona, Kubernetes and any third-party one.

src/pydantic_ai_backends/backends/daytona.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@
3232
DEFAULT_EXEC_TIMEOUT = 30 * 60
3333

3434

35-
class DaytonaSandbox(BaseSandbox): # pragma: no cover
35+
class DaytonaSandbox(BaseSandbox):
3636
"""Daytona cloud sandbox backend.
3737
3838
Creates an ephemeral Daytona sandbox for running commands and managing

src/pydantic_ai_backends/backends/kubernetes.py

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@
4040
DEFAULT_PORT = 8080
4141

4242

43-
class KubernetesPodSandbox(BaseSandbox): # pragma: no cover
43+
class KubernetesPodSandbox(BaseSandbox):
4444
"""Sandbox backed by a Kubernetes pod.
4545
4646
Args:
@@ -322,23 +322,37 @@ def _execute_api(self, command: str, timeout_seconds: int) -> ExecuteResponse:
322322
tty=False,
323323
_preload_content=False,
324324
)
325+
except Exception as exc:
326+
return ExecuteResponse(output=f"Error: {exc}", exit_code=1, truncated=False)
327+
328+
# `close` in `finally`: the read loop below can raise on a dropped
329+
# connection, and the websocket would then stay open for the life of the
330+
# process — one leaked socket per failed command.
331+
try:
325332
output = bytearray()
326333
while resp.is_open() and len(output) < MAX_EXECUTE_OUTPUT_BYTES:
327334
resp.update(timeout=1)
328335
if resp.peek_stdout():
329336
output.extend(resp.read_stdout().encode("utf-8", errors="replace"))
330337
if resp.peek_stderr():
331338
output.extend(resp.read_stderr().encode("utf-8", errors="replace"))
332-
exit_code = resp.returncode if resp.returncode is not None else 0
333-
resp.close()
334-
truncated = len(output) >= MAX_EXECUTE_OUTPUT_BYTES
335-
return ExecuteResponse(
336-
output=bytes(output[:MAX_EXECUTE_OUTPUT_BYTES]).decode("utf-8", errors="replace"),
337-
exit_code=exit_code,
338-
truncated=truncated,
339-
)
339+
# `1` for an unknown status, never `0`. The loop also exits once the
340+
# output cap is hit, with the command still running and no return
341+
# code yet — reporting that as success told the caller a truncated
342+
# command had passed. `LocalBackend` already defaults to `1`.
343+
exit_code = resp.returncode if resp.returncode is not None else 1
340344
except Exception as exc:
341345
return ExecuteResponse(output=f"Error: {exc}", exit_code=1, truncated=False)
346+
finally:
347+
with contextlib.suppress(Exception):
348+
resp.close()
349+
350+
truncated = len(output) >= MAX_EXECUTE_OUTPUT_BYTES
351+
return ExecuteResponse(
352+
output=bytes(output[:MAX_EXECUTE_OUTPUT_BYTES]).decode("utf-8", errors="replace"),
353+
exit_code=exit_code,
354+
truncated=truncated,
355+
)
342356

343357
def read_bytes(self, path: str) -> bytes:
344358
# Match LocalBackend semantics: return b"" on missing / transport /

tests/test_daytona_sandbox.py

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -456,3 +456,79 @@ def test_in_all(self) -> None:
456456
import pydantic_ai_backends
457457

458458
assert "DaytonaSandbox" in pydantic_ai_backends.__all__
459+
460+
461+
class TestDaytonaFailurePaths:
462+
"""Handlers that were hidden by the class-level pragma."""
463+
464+
def _sandbox(self):
465+
from pydantic_ai_backends import DaytonaSandbox
466+
467+
sandbox = DaytonaSandbox.__new__(DaytonaSandbox)
468+
sandbox._id = "s1"
469+
sandbox._last_activity = 0.0
470+
return sandbox
471+
472+
def test_is_alive_is_false_when_the_probe_fails(self, monkeypatch):
473+
sandbox = self._sandbox()
474+
monkeypatch.setattr(
475+
type(sandbox),
476+
"execute",
477+
lambda self, cmd, timeout=None: (_ for _ in ()).throw(OSError("transport gone")),
478+
)
479+
480+
assert sandbox.is_alive() is False
481+
482+
def test_is_alive_is_false_on_a_nonzero_exit(self, monkeypatch):
483+
from pydantic_ai_backends.types import ExecuteResponse
484+
485+
sandbox = self._sandbox()
486+
monkeypatch.setattr(
487+
type(sandbox),
488+
"execute",
489+
lambda self, cmd, timeout=None: ExecuteResponse(output="", exit_code=1),
490+
)
491+
492+
assert sandbox.is_alive() is False
493+
494+
def test_an_unexpected_failure_during_edit_is_reported(self, monkeypatch):
495+
sandbox = self._sandbox()
496+
monkeypatch.setattr(
497+
type(sandbox),
498+
# `edit` probes existence first, so that is where the failure lands.
499+
"exists",
500+
lambda self, path: (_ for _ in ()).throw(RuntimeError("api exploded")),
501+
)
502+
503+
result = sandbox.edit("/f.txt", "a", "b")
504+
505+
assert result.error is not None
506+
assert "api exploded" in result.error
507+
508+
def test_readiness_polling_tolerates_a_failing_probe(self, monkeypatch):
509+
"""A sandbox that is not up yet raises; that is expected, not fatal."""
510+
from pydantic_ai_backends.backends import daytona as module
511+
512+
sandbox = self._sandbox()
513+
attempts: list[int] = []
514+
515+
class Process:
516+
def exec(self, command: str, timeout: int = 5):
517+
attempts.append(1)
518+
if len(attempts) == 1:
519+
raise OSError("not listening yet")
520+
521+
class Ok:
522+
exit_code = 0
523+
524+
return Ok()
525+
526+
class Inner:
527+
process = Process()
528+
529+
sandbox._sandbox = Inner()
530+
monkeypatch.setattr(module.time, "sleep", lambda seconds: None)
531+
532+
sandbox._wait_until_ready(timeout=30)
533+
534+
assert len(attempts) == 2

0 commit comments

Comments
 (0)