Skip to content

Fix: reject a non-positive GkeCodeExecutor timeout - #198

Open
AmaadMartin wants to merge 2 commits into
mainfrom
fix/gke-executor-positive-timeout
Open

Fix: reject a non-positive GkeCodeExecutor timeout#198
AmaadMartin wants to merge 2 commits into
mainfrom
fix/gke-executor-positive-timeout

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    N/A
  2. Or, if no issue exists, describe the change:
    Problem: GkeCodeExecutor.timeout_seconds accepts 0 and negative values. That field is the only bound on the wait for a submitted Job, because the Job carries no active_deadline_seconds. A 0 does reach the watch request as timeoutSeconds=0, but the API server reads that as no timeout and applies its own default, so the executor then reports Job '...' did not complete within 0s. — a deadline that was never applied. A negative value is meaningless as a deadline.

Solution: The field is now Field(default=300, gt=0), so pydantic refuses a non-positive value at construction. This mirrors ContainerCodeExecutor, which already declares the same constraint. I used pydantic's own constraint instead of a field_validator because a numeric bound needs no custom code. Behavioural change: GkeCodeExecutor(timeout_seconds=0) and negative values now raise pydantic.ValidationError; the default stays 300 and positive values are unchanged.

Collision check: I listed the 100 open PRs on this fork and read the file list of every code-executor PR (100, 101, 106, 130, 132, 154, 157, 162, 164, 165, 174, 181). None touches gke_code_executor.py, and no branch on the fork carries this change.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

uv run pytest tests/unittests/code_executors/test_gke_code_executor.py -q — 20 passed (17 existing, 3 new parametrized cases: 0, -1, None). No existing test changed.

Mutation check: I replaced the new declaration with the old timeout_seconds: int = 300 and re-ran the new test. The 0 and -1 cases failed with Failed: DID NOT RAISE ValidationError, so they pin the new constraint. The None case still passed, because the bare int annotation already rejects it; it pins the narrowing the field docstring claims, and matches test_container_code_executor.py.

Coverage: the changed line is one field declaration and adds no branch. --cov --cov-branch reports it covered; the file total of 89% comes from pre-existing untested execution paths.

Checks: uv run mypy src/google/adk/code_executors/gke_code_executor.py reports no issues. pre-commit run --files <both files> passes isort, pyink, ruff and the license check.

Manual End-to-End (E2E) Tests:
No cluster is needed, because validation runs before any Kubernetes client is built. With uv sync --all-extras and no kubeconfig:

import pydantic
from google.adk.code_executors import GkeCodeExecutor

for bad in (0, -1, -300):
  try:
    GkeCodeExecutor(timeout_seconds=bad)
  except pydantic.ValidationError as e:
    print(bad, e.errors()[0]["msg"])

Each value prints Input should be greater than 0. A positive value passes validation and moves on to loading the kubeconfig.

CI state: all Unit Tests, A2A Tests and Mypy jobs pass. The Pre-commit Linter fails only on the update-constraints hook, which regenerates constraints-3.*.txt from today's date. That hook fails the same way on main at my base commit 352d11d3, and it touches no file in this diff.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

Amaad Martin added 2 commits August 9, 2026 13:02
`GkeCodeExecutor.timeout_seconds` had no lower bound. It is the only thing
that bounds the watch for a submitted Job, because the Job carries no
`active_deadline_seconds`. A value of `0` reaches the Kubernetes watch as
"no timeout supplied", so the API server applies its own much larger default
and the `did not complete within 0s` error names a deadline that was never
applied. A negative value is meaningless as a deadline.

The field now mirrors `ContainerCodeExecutor`: `Field(default=300, gt=0)`.
Behavioural change: constructing with `0` or a negative value now raises
`pydantic.ValidationError`. The default stays 300 and positive values are
unaffected.
Correct the attribution in the field docstring: the generated Kubernetes
client guards on `is not None`, so it does put `timeoutSeconds=0` on the
wire. The API server, not the client, reads that zero as "no timeout".
Drop the sentences that restate the `gt=0` constraint or point at a
private method.

Add `None` to the rejection test, so the docstring's claim that `None` is
rejected is pinned. This matches `test_container_code_executor.py`.
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