Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

restart_wrapper: Be explicit about what signals are forwarded to entr #626

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jirwin
Copy link

@jirwin jirwin commented Jan 11, 2025

Background

In #586 support for forwarding all signals sent to PID 1 on to entr was added. However, it doesn’t fully address the goal of #577, since the developer’s "true" entrypoint isn’t receiving those forwarded signals—only entr is. While this generally works for shutting down child processes, it can also forward unintended signals that break entr.

A concrete example is when the child process managed by entr becomes a zombie and gets re-parented to PID 1. The kernel then sends SIGCHLD to PID 1, which in turn forwards it to entr. These "spurious" SIGCHLD signals can cause entr to stop monitoring or restarting the process, breaking the restart_wrapper functionality.

How to Reproduce

  1. kubectl exec into a pod that uses restart_wrapper.
  2. Run kill -SIGCHLD <entr-pid>.
  3. After this, Tilt will detect file changes and run live update logic, but the process never actually restarts.

Fix

To keep the spirit of #586 this PR changes the code so that only SIGKILL, SIGINT, and SIGTERM are forwarded. This prevents unintended signals (like SIGCHLD) from being passed to entr, avoiding the issue where entr stops managing its child processes after receiving unexpected signals.

@jirwin jirwin force-pushed the jirwin/restart-wrapper-explicit-signals branch from 65d769b to ca1db42 Compare January 11, 2025 03:44
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