Fix: report an unusable spawn interpreter from UnsafeLocalCodeExecutor instead of leaking BrokenPipeError - #165
Open
AmaadMartin wants to merge 3 commits into
Open
Conversation
added 3 commits
August 7, 2026 16:59
UnsafeLocalCodeExecutor starts its worker by re-invoking the interpreter multiprocessing holds. When that interpreter cannot be exec'd, the queue or the worker start raises BrokenPipeError out of execute_code, which is declared to return a CodeExecutionResult and whose callers do not catch. execute_code now returns a result whose stderr names the interpreter multiprocessing would re-invoke, states that no code ran, and points at the remediation. The traceback is still logged at ERROR.
Adds eight tests: an end-to-end exercise of the reported bug in a child interpreter, the falsy-interpreter guard, both OSError origins, the queue release, the absent teardown, the narrow catch and the logged traceback. The end-to-end test runs in a child interpreter because the launch failure only surfaces while the multiprocessing resource tracker has not started yet, and because it must not leave a dead tracker behind in the pytest worker.
Whether multiprocessing reports an unusable interpreter at all is a race: the parent learns of the failure only when its write to the dead child's pipe returns EPIPE. The child-interpreter version of this test therefore timed out on Python 3.13 in CI while passing on the other four versions. The test now sets the interpreter path through the real multiprocessing.set_executable() and injects the OSError whose origin is racy, so it pins the same two assertions without the race.
This was referenced Aug 8, 2026
Open
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.
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
N/A — no public issue tracks this defect.
Problem:
UnsafeLocalCodeExecutorstarts its worker by re-invoking the Python interpreter thatmultiprocessingholds. When that interpreter cannot be exec'd,execute_codeleaksBrokenPipeError: [Errno 32] Broken pipeout ofmultiprocessing/resource_tracker.py, which reads like a fault in the executed snippet. The method is declared to return aCodeExecutionResultand the flow processor does not catch, so the whole invocation aborts. The model then retries the same snippet and fails the same way, because nothing in the error names the interpreter.Solution:
execute_codenow returns a result whosestderrnames the interpreter, states that no code was executed, and gives the remediation. The message readsmultiprocessing.spawn.get_executable(), neversys.executable: those two differ exactly when a host has calledmultiprocessing.set_executable(), which is the situation being diagnosed. The catch stays onOSError, so a non-OSErrorsetup failure still propagates, and an interpreter path that is falsy is caught by a guard before any multiprocessing object exists. The traceback is logged once atERROR, so operators keep the cause.Reporting in
stderrfollows the peer executors:GkeCodeExecutorreports infrastructure errors that way, and this executor already reports its own timeout that way.Out of scope, and left alone: a worker that starts and then dies without a result. That is a different failure with its own open pull request.
Collision check: I listed the open pull requests on this fork and read the diff of every one that touches this file. Three of them change the same method. They form a stack whose middle part adds a public exception class that the tip deletes, they are based on a
mainthat predates_kill_executionand the strict-mypy annotations, and their message namessys.executable. This change starts from currentmain, is self-contained, and does not carry that defect.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.
Eight new tests. The eleven existing tests are unmodified and still pass.
test_execute_code_reports_the_unusable_interpreter— the reported bug: a host has pointedmultiprocessingat a binary that is not there. It uses the realmultiprocessing.set_executable(), and asserts that the bogus path appears instderrand thatsys.executabledoes not.test_execute_code_reports_a_missing_interpreter_path— the falsy-interpreter guard, driven through the realmultiprocessingglobal state with no test double at all.OSErrorout ofProcess.start()and out ofQueue(), the two origins the failure has...._releases_the_queue_when_the_worker_cannot_start— a recorder over a real spawn queue provesclose()andjoin_thread()each run once...._does_not_tear_down_a_worker_that_never_started— noterminate(),join()orkill()on a worker that never started...._propagates_a_non_oserror_setup_failure— pins the narrow catch...._logs_the_launch_failure— oneERRORrecord, withexc_info.Tests 3 to 8 inject a fake spawn context, because whether
multiprocessingreports an unusable interpreter at all is a race, and a test cannot depend on it.multiprocessinglearns the interpreter is unusable only when the write to a dead child's pipe returnsEPIPE, so if the parent wins the race against the failingexec, no error is raised and the parent waits on a queue no worker will fill. I hit this: a first revision drove the real stack in a child interpreter, passed on Python 3.10, 3.11, 3.12 and 3.14, and timed out on 3.13 in CI while passing five times out of five on 3.13 locally. The tests now pin the interpreter through the realmultiprocessing.set_executable()and inject only theOSErrorwhose origin is racy.That race is the neighbouring defect, not this one: when
multiprocessinghides the failure, the worker dies and the parent waits, which is the out-of-scope case named above.Coverage of the changed module is 100% line and branch on the new code (
pytest --cov=google.adk.code_executors.unsafe_local_code_executor --cov-branch). The 21 lines the report still marks as missed are all pre-existing: they run inside the spawned child, or in error branches of the pre-existing teardown helpers.Each new test was run against broken code and observed to fail:
BrokenPipeError: [Errno 32] Broken pipeescapes. Test 7 passes, since it pins a catch the old code does not have.except OSError→except ExceptionFailed: DID NOT RAISE ValueErrorclose()/join_thread()assert [] == ['close', 'join_thread']logger.exception(...)assert 0 == 1interpreter = sys.executableBrokenPipeError: [Errno 32] Broken pipeCI runs the unit tests and mypy green on Python 3.10 to 3.14. The
Pre-commit Linterjob is red for a reason that predates this branch: theupdate-constraintshook rewrites the fiveconstraints-3.*.txtfiles, which this change does not touch. That job is red on the other open pull requests on this fork too.Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.
On a POSIX host, from a checkout with the change applied:
Before the change the command dies with
BrokenPipeError: [Errno 32] Broken pipe. After it, nothing escapes and the printedstderrnames/definitely/not/a/real/python, says no code was executed, and points atContainerCodeExecutor/VertexAiCodeExecutor.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.