Skip to content

Leave shell as PID 1 - #214

Open
zregvart wants to merge 2 commits into
sol1:mainfrom
zregvart:pr/exec-no-trap
Open

zregvart wants to merge 2 commits into
sol1:mainfrom
zregvart:pr/exec-no-trap

Conversation

@zregvart

@zregvart zregvart commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

When rustguac replaces the PID 1 the shell will never invoke the trap on exit. This leaves the shell as PID 1 for the trap to be invoked on exit.

When rustguac replaces the PID 1 the shell will never invoke the trap
on exit. This leaves the shell as PID 1 for the trap to be invoked
on exit.

@davekempe davekempe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting this. The underlying observation is correct: with exec, the shell is replaced, so the trap ... TERM INT never fires and guacd is never signalled on shutdown.

I do not think this is quite the right fix though, and as written it regresses shutdown behaviour.

The & is missing from the rustguac line, so $! still refers to the last process that was actually backgrounded, which is guacd:

guacd ... &
GUACD_PID=$!             # guacd
...
rustguac ... serve       # foreground, never backgrounded
RUSTGUAC_PID=$!          # still guacd
wait $RUSTGUAC_PID

So RUSTGUAC_PID holds guacd's PID. The new trap then kills guacd twice and never signals rustguac at all. On docker stop that means rustguac receives no SIGTERM, runs until Docker's timeout and is then SIGKILLed, which is worse than the current exec behaviour where rustguac is PID 1 and gets SIGTERM directly.

For the background-and-wait pattern to work, rustguac needs a real &:

rustguac ... serve &
RUSTGUAC_PID=$!
wait "$RUSTGUAC_PID"

More broadly, for a container running two processes a real init is a better answer than hand-rolled signal forwarding, since it gets reaping and propagation right without the shell having to:

ENTRYPOINT ["/usr/bin/tini", "--", "/opt/rustguac/entrypoint.sh"]

Happy to take either shape. If you want to keep the shell supervisor then the & is the minimum fix; if you would rather go the init route, that would be very welcome.

Adds tini to manage processes launched from entrypoint.sh, namely guacd
and rustguac.
@zregvart

Copy link
Copy Markdown
Contributor Author

Thanks for the feedback, opted to go with tini, have a look at the changes pushed in 2216317.

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.

2 participants