client: actually terminate the tunnel-encap Envoy subprocess#1561
Merged
Conversation
oschaaf
force-pushed
the
encap-subprocess-termination
branch
2 times, most recently
from
733ad0d to
2d49aeb
Compare
EncapsulationSubProcessRunner::RunWithSubprocess() declared a local pid_t pid_ that shadowed the class member, so the member stayed -1 and TerminateEncapSubProcess() always returned early without signaling the child: the encap Envoy subprocess was never terminated or reaped on shutdown or cancellation, and was orphaned on every tunnel-mode run. Assign the fork() result to the member instead. The shadowing bug was also masking a second defect: RunWithSubprocess() ended by calling TerminateEncapSubProcess() right after nighthawk_fn() returns - but nighthawk_fn() only starts execution, so with the member populated this would have torn down the tunnel at the start of the load test. Remove that call; termination is owned by the existing call sites that now work: ProcessImpl::shutdown(), requestExecutionCancellation(), and the runner's destructor as a last resort. Two hardening changes that reaping made necessary: the forked child now exits via _exit() instead of exit(), because running atexit/sanitizer hooks in a fork of a multithreaded parent (e.g. the gRPC service) can deadlock on locks owned by threads that do not exist after fork - and a wedged child would now wedge the parent's waitpid() too. And TerminateEncapSubProcess() reaps with a bounded wait (SIGTERM, ~15s of WNOHANG polling, then SIGKILL) instead of an unbounded waitpid(). Signed-off-by: Otto van der Schaaf <oschaaf@we-amp.com>
oschaaf
force-pushed
the
encap-subprocess-termination
branch
from
2d49aeb to
8c78a23
Compare
eric846
approved these changes
Jul 9, 2026
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
2 participants
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.
Found during an adversarial review of the shutdown-sequence fixes (a
-Wshadowclass of bug).EncapsulationSubProcessRunner::RunWithSubprocess()declared a localpid_t pid_ = fork();shadowing the class member, so the member stayed-1andTerminateEncapSubProcess()always returned early without signaling the child: the encap Envoy subprocess was never terminated or reaped on shutdown or cancellation, and was orphaned on every tunnel-mode run.Fix: assign the
fork()result to the member. The shadowing bug was also masking a second defect:RunWithSubprocess()ended by callingTerminateEncapSubProcess()right afternighthawk_fn()returns — butnighthawk_fn()only starts execution, so with the member populated this would have torn down the tunnel at the start of the load test. That call is removed; termination is owned by the existing call sites that now actually work:ProcessImpl::shutdown(),requestExecutionCancellation(), and the runner's destructor as a last resort.Testing:
TestWithEncapsulation(forks a real encap Envoy) behaves identically to main on the machine used for verification, including an environment-specific IPv4 flake that reproduces at the same rate (1-in-3) on unmodified main.Open design question for review:
ProcessImpl::shutdown()terminates the encap subprocess before draining the workers, so tunnel-mode drains will now see connection resets rather than graceful completions. Arguably fine for shutdown, but moving termination after the worker joins would preserve graceful drains; left untouched here to keep this PR minimal.Update (asan CI failure on the first revision):
test_remote_execution.pyfailed under asan — the first execution succeeded, then the service stopped answering. Root cause analysis:RunWithSubprocess()forks on every execution (pre-existing, tunnel or not), and the child exited viaexit(0)— full C++ exit whose atexit/sanitizer hooks can deadlock in a fork of the multithreaded gRPC service (only the forking thread survives a fork). Before this PR nobodywaitpid()ed, so a wedged child leaked invisibly; with reaping in place it wedged the service's shutdown. Two hardening changes: the child now exits via_exit(0), andTerminateEncapSubProcess()reaps with a bounded wait (SIGTERM, ~15s WNOHANG polling, then SIGKILL) so a stuck child can never hang the caller.