Skip to content

Fix: make the process-group teardown check in test_unsafe_local_code_executor deterministic - #181

Open
AmaadMartin wants to merge 4 commits into
mainfrom
fix/deterministic-kill-execution-liveness-check
Open

Fix: make the process-group teardown check in test_unsafe_local_code_executor deterministic#181
AmaadMartin wants to merge 4 commits into
mainfrom
fix/deterministic-kill-execution-liveness-check

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 8, 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 - no public issue.
  2. Or, if no issue exists, describe the change:

Problem: tests/unittests/code_executors/test_unsafe_local_code_executor.py::TestUnsafeLocalCodeExecutor::test_kill_execution_kills_what_the_code_spawned fails intermittently. Its _is_alive() helper treated only Z as a post-mortem /proc state, but Linux also reports X (x on older kernels) for a process that has been reaped and is being torn down. The wait loop stopped on the reading that showed the fork was dead, and then the assertion read /proc a second time; if the fork moved Z -> X in that gap, the assertion called a dead process alive and failed. The same helper raised IndexError instead of answering, if the entry vanished between open() and read().

Solution: _is_alive() now knows every post-mortem state and treats a vanished entry as not alive. The assertion consumes the reading the wait settled on, through a _has_exited() helper, so no second sample can overturn it. This is sound because a process that has exited does not come back. Both deadlines use time.monotonic(), matching test_container_code_executor.py.

The test keeps its full strength. It still spawns a real execution process, still makes the executed code os.fork() a real grandchild, still calls the real _kill_execution(), and still fails if the grandchild survives. It also now asserts the fork was running before the kill, so the check cannot pass vacuously. src/google/adk/code_executors/unsafe_local_code_executor.py is unchanged, and no other test in the file is touched.

Deviation from the plan I worked from: the plan wrote the corrected wait inline in the test body. Inline, the loop body is only reached when the fork takes more than one poll to die, so it cannot be covered deterministically and the settled-reading rule has no test of its own. I extracted _has_exited() instead and covered both.

Collision check: gh pr list --repo AmaadMartin/adk-python --state open --limit 100 returned 100 open PRs. Eight touch this file (#164, #165, #130, #106, #101, #100, #83, #81). I diffed each for _is_alive, "Z", rsplit, rpartition and the state set: none changes the predicate, so none lands this fix. #101 is the closest - it moves the same wait into an _await_death() helper, but keeps both defects (Z-only, and a second read after the loop). I did not stack on it: it also rewrites unsafe_local_code_executor.py, and this change must stay test-only. Whichever merges second resolves a small conflict in one function.

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.

Four new tests, all deterministic and all at module level next to the helpers they cover:

  • test_only_a_process_that_has_not_exited_counts_as_alive - a table over R, S, D, Z, X, x, a name containing parentheses, and an empty read.
  • test_a_pid_with_no_proc_entry_is_not_alive - the OSError path.
  • test_the_wait_keeps_the_reading_it_settled_on - a scripted probe returns alive, dead, alive; the wait must report the second reading.
  • test_the_wait_gives_up_when_the_process_outlives_the_deadline - a live process and a zero budget.

Proof the tests can fail. Each mutation was applied to the source, run, then reverted.

# Mutation Test that FAILED Message
M1 _DEAD_STATES -> frozenset("Z") (the bug) the table, X and x rows assert True is False where True = _shows_a_live_process('4321 (python3) x 0 -1 -1 0 -1')
M2 drop the empty-fields guard the table, "" row IndexError: list index out of range
M3 rpartition(")") -> partition(")") the table, (py (3)) Z row assert True is False where True = _shows_a_live_process('4321 (py (3)) Z 1 4321 4321 0 -1')
M4 restore the pre-fix wait shape (loop, then re-read) test_the_wait_keeps_the_reading_it_settled_on assert False where False = _has_exited(4321, 10)
M5 return exited -> return True test_the_wait_gives_up_when_the_process_outlives_the_deadline assert not True where True = _has_exited(1159762, 0)
M6 except OSError: return False -> return True test_a_pid_with_no_proc_entry_is_not_alive assert not True where True = _is_alive(-1)

Coverage of the changed file, measured with pytest --cov-branch: 96%, up from 95% on main. Every line and branch this PR adds is covered. The four remaining misses are pre-existing and untouched: the pytest.skip() escape hatch, and three lines of the integration test's finally cleanup that only run when the test has already failed.

Manual End-to-End (E2E) Tests

Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

Everything below ran on the pushed commit, Linux, Python 3.14.4.

$ uv run pytest tests/unittests/code_executors/test_unsafe_local_code_executor.py -q
23 passed, 1 warning in 13.05s

$ for i in $(seq 1 20); do uv run pytest tests/unittests/code_executors/test_unsafe_local_code_executor.py -q || echo "FAILED on run $i"; done
0 failures in 20 runs

$ for i in $(seq 1 6); do uv run pytest tests/unittests/code_executors -q -n auto; done
122 passed each round; 0 failures in 6 rounds

$ uv run pre-commit run --files tests/unittests/code_executors/test_unsafe_local_code_executor.py
isort Passed / pyink Passed / addlicense Passed / ADK Compliance Checks Passed / codespell Passed

Honest limit on the reproduction. I could not make the flake fire on this machine: main also passed 20/20 file runs and 8 rounds of pytest tests/unittests/code_executors -n auto, and a standalone harness running the pre-fix wait-and-re-read against a real killed grandchild passed 3600 rounds. So the local runs above confirm no regression; they do not by themselves prove the race is gone.

What I did measure directly, with a harness that kills a setsid group the same way the executor does and then records every /proc state the grandchild passes through:

280  R->Z->gone
 19  R->Z->X->gone       <- 6.3% of teardowns reach X
  1  R->D->R->Z->gone

and the state windows, over another 300 kills:

Z window: n=300 median=317us max=553us
X window: n=27  median=0us    max=0us

So X is reached routinely, and _is_alive() called it alive (M1 proves that). The rate depends on how far apart the machine puts the two reads: here they are microseconds apart against a ~317 us Z window, which is why 3600 rounds were not enough; under pytest -n auto on a saturated runner the second read can slip a whole scheduling quantum, which is when it lands in X. The fix removes the second read, so the rate no longer matters.

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.

CI

Unit Tests (Python 3.10-3.14), Mypy Check (3.10-3.13) and A2A v0.3 Tests (3.10-3.14) all pass.

Pre-commit Linter is red, and it is red on the base commit too. Run 31226527302 on 352d11d3 — the commit this branch is based on, with no changes of mine — fails the same job while every other job passes. The hook is update-constraints: it recompiles constraints-*.txt against today's date and reports the header dates as a diff. This PR touches no dependency file. Open PRs #163 and #149 address that hook.

Amaad Martin added 4 commits August 8, 2026 02:01
…stic

The liveness predicate in test_unsafe_local_code_executor.py treated only "Z"
as a post-mortem /proc state. Linux also reports "X" ("x" on older kernels) for
a process that has been reaped but whose /proc entry is still being torn down,
so the predicate called an exited process alive. The teardown wait then made
that reachable: the loop stopped on the reading that showed the fork was dead,
and the assertion threw that reading away and sampled /proc again.

The predicate now knows every post-mortem state and treats a vanished entry as
not alive instead of raising IndexError, and the assertion consumes the reading
the wait settled on. Both deadlines use a monotonic clock. A new table test
covers the predicate for each state.
The wait that the assertion consumes is now a named helper, so the rule it
enforces -- the verdict is the reading the wait settled on -- has a
deterministic test instead of depending on a real process losing a race.
A second test pins the not-alive answer for a pid with no /proc entry.
pytest resolves a dotted target itself, so the test no longer reaches into
sys.modules for its own module object, and the sys import goes with it.
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