UN-3098 [FIX] Fix SIGTERM trap handlers for graceful container shutdown#1752
Merged
UN-3098 [FIX] Fix SIGTERM trap handlers for graceful container shutdown#1752
Conversation
- Remove dumb-init from tool container command to prevent signal forwarding to process group (was causing premature termination) - Fix sidecar SIGTERM handler to print to stdout instead of writing to shared log file (was causing permission denied errors) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Contributor
|
Caution Review failedThe pull request is closed. Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughRemoved dumb-init wrapper from container execution command and updated the sidecar's SIGTERM handler to log to stdout instead of file, with exec removed to allow trap execution. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
|
ritwik-g
changed the title
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
1 participant
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.
Summary
dumb-initfrom tool container command to prevent signal forwarding to process groupProblem
The previous SIGTERM trap implementation had two issues:
dumb-initforwards SIGTERM to the entire process group, so Python was still receiving SIGTERM and terminating despite the shell trap/shared/logs/logs.txtcaused "Permission denied" errors since the sidecar only has read access to that fileSolution
dumb-initso the shell is PID 1 and can properly intercept SIGTERMTest plan
🤖 Generated with Claude Code