fix: Inproper signal handling #5131
Conversation
mowirth
requested review from
ThisGuyCodes,
denis256 and
yhakbar
as code owners
|
@mowirth is attempting to deploy a commit to the Gruntwork Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughConfigure command cancellation to send SIGINT to child processes (instead of forceful termination) by setting Cancel hooks on exec.Cmd in multiple execution layers and updating tests to assert the new signal-based behavior. No public APIs changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.go⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (3)📚 Learning: 2025-02-10T13:36:19.542ZApplied to files:
📚 Learning: 2025-04-17T13:02:28.098ZApplied to files:
📚 Learning: 2025-08-19T16:05:54.723ZApplied to files:
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 |
|
I have some issues running the test locally as I have not installed all dependencies, so I'll be happy if you could give them a run and check if something is not in order. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
shell/run_cmd_windows_test.go (1)
49-69: Update comments to match new expectations (no longer acceptsignal: killed)The assertions now reject
"signal: killed"and only allow"Failed to execute \"%s"with either"exit status 5"or"exit status 1", which is aligned with the desired graceful termination behavior. However, the surrounding comments still say the process “might … be killed by a signal,” which contradicts the checks.Consider updating the comments to describe the new, stricter expectation (exit status–based failures only) so they don’t mislead future readers.
internal/os/exec/cmd.go (2)
32-45:Commandno longer usesctx→ compile error and potential behavioral ambiguity
Commandkeeps actx context.Contextparameter but no longer references it inside the function body after switching toexec.Command(name, args...). In Go this will fail to compile (ctx declared but not used) and also makes it unclear whetherctxis still meant to influence process lifetime.If you intend to decouple the underlying
exec.Cmdfrom context cancellation and rely solely onRegisterGracefullyShutdown, you still need to usectxto avoid the compile error — for example:func Command(ctx context.Context, name string, args ...string) *Cmd { - cmd := &Cmd{ + // ctx is intentionally not wired into exec.Command; it is used only for higher-level shutdown logic. + _ = ctx + cmd := &Cmd{ Cmd: exec.Command(name, args...), logger: log.Default(), filename: filepath.Base(name), interruptSignal: signal.InterruptSignal, }Alternatively (and more semantically meaningful), you could store
ctxonCmdor call a registration hook here, but that requires checking all call sites to avoid double registration.
4-18: GracefulCommandContext uses Unix-only syscalls without OS guards — confirmed issueThe review comment is accurate. The
GracefulCommandContextfunction (lines 133–148) directly uses platform-specific code without build constraints:
- Line 137:
cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}—Setpgidis Unix-only- Line 143:
syscall.Kill(-cmd.Process.Pid, signal.InterruptSignal)— negative PID process-group semantics are Unix-only- Windows signal:
InterruptSignal = nil(will cause runtime errors when passed tosyscall.Kill)The parent file has no
//go:buildconstraints, meaning this code compiles on Windows despite being non-functional there. The function is called in production code paths (engine, git operations, format command).The codebase already uses OS-specific files with proper build constraints elsewhere (
ptty_unix.go/ptty_windows.go), establishing the pattern. Creatingcmd_unix.gowith//go:build !windowsfor this function and acmd_windows.gofallback is the appropriate fix.
🧹 Nitpick comments (1)
test/helpers/test_helpers.go (1)
14-15: Tests now benefit from process‑group‑aware cancellationSwitching
ExecWithTestLoggertointernalExec.GracefulCommandContextaligns test subprocess handling with the new helper and should give more realistic interrupt behavior for nested processes.You may optionally consider migrating
ExecAndCaptureOutputto use the same helper for consistent cancellation semantics across all test helpers, but this is not required for correctness.Also applies to: 75-100
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cli/commands/hcl/format/format.go(2 hunks)engine/engine.go(2 hunks)internal/cas/git.go(2 hunks)internal/os/exec/cmd.go(3 hunks)shell/run_cmd_unix_test.go(1 hunks)shell/run_cmd_windows_test.go(1 hunks)test/helpers/test_helpers.go(2 hunks)util/shell.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
Files:
internal/cas/git.gotest/helpers/test_helpers.gocli/commands/hcl/format/format.gointernal/os/exec/cmd.goshell/run_cmd_windows_test.goshell/run_cmd_unix_test.goengine/engine.goutil/shell.go
🧠 Learnings (4)
📓 Common learnings
Learnt from: jorhett
Repo: gruntwork-io/terragrunt PR: 1234
File: errors/multierror.go:35-39
Timestamp: 2025-09-10T04:41:35.652Z
Learning: In Terragrunt's MultiError.Error() method, checking for the exact string "exit status 2" and returning it directly is not a brittle hack but a semantic fix. Terraform's --detailed-exitcode flag uses exit code 2 to mean "plan succeeded with changes" (not an error), so when multiple modules return this status, it should not be wrapped in "Hit multiple errors:" formatting as that misrepresents successful operations as errors.
📚 Learning: 2025-02-10T13:36:19.542Z
Learnt from: levkohimins
Repo: gruntwork-io/terragrunt PR: 3723
File: cli/commands/stack/action.go:160-160
Timestamp: 2025-02-10T13:36:19.542Z
Learning: The project uses a custom error package `github.com/gruntwork-io/terragrunt/internal/errors` which provides similar functionality to `fmt.Errorf` but includes stack traces. Prefer using this package's error functions (e.g., `errors.Errorf`, `errors.New`) over the standard library's error handling.
Applied to files:
internal/cas/git.gotest/helpers/test_helpers.gocli/commands/hcl/format/format.gointernal/os/exec/cmd.goengine/engine.goutil/shell.go
📚 Learning: 2025-04-17T13:02:28.098Z
Learnt from: yhakbar
Repo: gruntwork-io/terragrunt PR: 4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
Applied to files:
internal/cas/git.gotest/helpers/test_helpers.gointernal/os/exec/cmd.goengine/engine.goutil/shell.go
📚 Learning: 2025-08-19T16:05:54.723Z
Learnt from: Resonance1584
Repo: gruntwork-io/terragrunt PR: 4683
File: go.mod:86-90
Timestamp: 2025-08-19T16:05:54.723Z
Learning: When analyzing Go module dependencies for removal, always check for both direct imports and API usage across all Go files in the repository, not just a quick search. The github.com/mattn/go-zglob library is used for filesystem walking and glob expansion in multiple Terragrunt files including util/file.go, format commands, and AWS provider patch functionality.
Applied to files:
cli/commands/hcl/format/format.gointernal/os/exec/cmd.goengine/engine.goutil/shell.go
🧬 Code graph analysis (4)
internal/cas/git.go (1)
internal/os/exec/cmd.go (1)
GracefulCommandContext(133-148)
test/helpers/test_helpers.go (1)
internal/os/exec/cmd.go (1)
GracefulCommandContext(133-148)
cli/commands/hcl/format/format.go (1)
internal/os/exec/cmd.go (1)
GracefulCommandContext(133-148)
engine/engine.go (1)
internal/os/exec/cmd.go (1)
GracefulCommandContext(133-148)
🔇 Additional comments (6)
util/shell.go (1)
3-13: Import placement change is safeMoving
os/execinto the main import block is purely organizational and has no behavioral impact; usage in this file remains unambiguous.cli/commands/hcl/format/format.go (1)
21-34: GracefulCommandContext usage inbytesDifflooks correctUsing
internal/os/exec.GracefulCommandContextfor thediffsubprocess preserves existing behavior while making cancellation process‑group‑aware and more graceful. Arguments and error handling are unchanged.Also applies to: 262-310
internal/cas/git.go (2)
13-14: LGTM! Clean import alias for internal exec package.The import alias
internalExecis clear and avoids shadowing the standardos/execpackage that's still needed forexec.LookPathat line 29.
285-285: LGTM! Correct integration with GracefulCommandContext.The call properly constructs the command by passing the git executable path and combining the subcommand (
name) with its arguments. This ensures git operations receive proper interrupt signals via process groups, addressing the state-locking issue described in #4839.engine/engine.go (2)
17-17: LGTM! Import updated to use internal exec wrapper.The import change enables graceful signal handling for the engine subprocess, which is essential for proper Terraform state management during interrupts.
502-502: LGTM! Engine process now uses graceful signal handling.The switch to
GracefulCommandContextensures the engine plugin receives proper interrupt signals, allowing it to clean up resources and unlock state before termination. This is critical for the gRPC plugin lifecycle managed by hashicorp/go-plugin.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shell/run_cmd_unix_test.go (1)
49-50: Remove or update the outdated comment.The comment states the process "might either exit with the expected status code or be killed by a signal," but the test code explicitly treats being killed as a failure (lines 54-56). This contradicts the PR's objective of fixing signal handling to ensure graceful termination. The comment appears to describe the old (buggy) behavior rather than the new expected behavior.
Apply this diff to remove the outdated comment:
- // The process might either exit with the expected status code or be killed by a signal - // depending on timing and system conditions expectedExitStatusErr := fmt.Sprintf("Failed to execute \"%s 5\" in .\n\nexit status %d", cmdPath, expectedWait)
🧹 Nitpick comments (1)
shell/run_cmd_unix_test.go (1)
55-55: Consider improving error message formatting for consistency.The error message formatting
"but got\n: %s"produces a newline followed by a colon, which results in:Expected process to gracefully terminate but got : signal: killedFor consistency with the other error message (lines 59-60) and better readability, consider using the pattern
"but got:\n %s":- t.Errorf("Expected process to gracefully terminate but got\n: %s", actualErr.Error()) + t.Errorf("Expected process to gracefully terminate but got:\n %s", actualErr.Error())
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
shell/run_cmd_unix_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
Files:
shell/run_cmd_unix_test.go
🧠 Learnings (1)
📚 Learning: 2025-09-10T04:41:35.652Z
Learnt from: jorhett
Repo: gruntwork-io/terragrunt PR: 1234
File: errors/multierror.go:35-39
Timestamp: 2025-09-10T04:41:35.652Z
Learning: In Terragrunt's MultiError.Error() method, checking for the exact string "exit status 2" and returning it directly is not a brittle hack but a semantic fix. Terraform's --detailed-exitcode flag uses exit code 2 to mean "plan succeeded with changes" (not an error), so when multiple modules return this status, it should not be wrapped in "Hit multiple errors:" formatting as that misrepresents successful operations as errors.
Applied to files:
shell/run_cmd_unix_test.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Pull Request has non-contributor approval
- GitHub Check: Pull Request has non-contributor approval
|
@denis256 @ThisGuyCodes @yhakbar Could you please take a look? |
|
Failed on lint |
|
@sylr thanks for the help, I integrated it in in my branch. Regarding the lint errors related to the CommandContext, I don't think there is a way around this - as expained above, CommandContext will always send a SIGKILL to the underlying process. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/os/exec/cmd.go (1)
32-34: Unusedctxparameter after switching toexec.Command.The
ctxparameter is no longer used in this function since the switch fromexec.CommandContexttoexec.Command. This will also trigger thenoctxlinter (as noted in PR comments).Consider either:
- Adding
_ = ctxor a lint ignore directive with a comment explaining the intentional design, or- Removing the
ctxparameter if callers can obtain context-aware behavior viaRegisterGracefullyShutdowninstead (though this is a breaking API change).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
engine/engine.go(2 hunks)internal/os/exec/cmd.go(3 hunks)util/shell.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- util/shell.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
Files:
internal/os/exec/cmd.goengine/engine.go
🧠 Learnings (3)
📚 Learning: 2025-02-10T13:36:19.542Z
Learnt from: levkohimins
Repo: gruntwork-io/terragrunt PR: 3723
File: cli/commands/stack/action.go:160-160
Timestamp: 2025-02-10T13:36:19.542Z
Learning: The project uses a custom error package `github.com/gruntwork-io/terragrunt/internal/errors` which provides similar functionality to `fmt.Errorf` but includes stack traces. Prefer using this package's error functions (e.g., `errors.Errorf`, `errors.New`) over the standard library's error handling.
Applied to files:
internal/os/exec/cmd.goengine/engine.go
📚 Learning: 2025-08-19T16:05:54.723Z
Learnt from: Resonance1584
Repo: gruntwork-io/terragrunt PR: 4683
File: go.mod:86-90
Timestamp: 2025-08-19T16:05:54.723Z
Learning: When analyzing Go module dependencies for removal, always check for both direct imports and API usage across all Go files in the repository, not just a quick search. The github.com/mattn/go-zglob library is used for filesystem walking and glob expansion in multiple Terragrunt files including util/file.go, format commands, and AWS provider patch functionality.
Applied to files:
internal/os/exec/cmd.goengine/engine.go
📚 Learning: 2025-04-17T13:02:28.098Z
Learnt from: yhakbar
Repo: gruntwork-io/terragrunt PR: 4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
Applied to files:
internal/os/exec/cmd.goengine/engine.go
🧬 Code graph analysis (1)
engine/engine.go (1)
internal/os/exec/cmd.go (1)
GracefulCommandContext(133-149)
🔇 Additional comments (1)
engine/engine.go (1)
17-17: LGTM - Proper adoption of graceful shutdown helper.The switch to
GracefulCommandContextcorrectly enables the engine process to receive interrupt signals when the context is cancelled, supporting the goal of allowing Terraform to properly handle interrupts and unlock state.Also applies to: 502-502
internal/os/exec/cmd.go
Outdated
| func GracefulCommandContext(ctx context.Context, name string, args ...string) *exec.Cmd { | ||
| cmd := exec.Command(name, args...) | ||
|
|
||
| // Put subprocess in its own process group | ||
| cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} | ||
|
|
||
| go func() { | ||
| <-ctx.Done() | ||
|
|
||
| if cmd.Process != nil { | ||
| // Send the provided signal to the process group | ||
| _ = syscall.Kill(-cmd.Process.Pid, signal.InterruptSignal) | ||
| } | ||
| }() | ||
|
|
||
| return cmd |
There was a problem hiding this comment.
Platform portability issue: Unix-only syscall usage.
syscall.SysProcAttr{Setpgid: true} and syscall.Kill(-pid, ...) with negative PID are Unix-specific and will cause compilation failures on Windows. Given the PR notes testing on Windows 11, this needs platform-specific handling.
Consider moving this function to a cmd_unix.go file with build constraints, and providing a Windows-compatible stub (or alternative implementation) in cmd_windows.go:
-func GracefulCommandContext(ctx context.Context, name string, args ...string) *exec.Cmd {
+// cmd_unix.go
+//go:build unix
+
+func GracefulCommandContext(ctx context.Context, name string, args ...string) *exec.Cmd {Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
internal/os/exec/cmd.go around lines 133-148 uses Unix-only syscalls
(SysProcAttr.Setpgid and negative-PID syscall.Kill) which will break Windows
builds; move this implementation into a new file cmd_unix.go with a build tag
"//go:build !windows" and keep the same GracefulCommandContext signature and
behavior there, restrict imports to syscall in that file only, and create a
cmd_windows.go with build tag "//go:build windows" that provides a
Windows-compatible stub/alternative implementation (same function signature)
that does not use Unix syscalls (e.g., omit Setpgid and send signals using
os.Process.Signal or Process.Kill as appropriate) so the package builds on both
platforms.
|
I think you can just do something like: The CommandContext function is: So you can just override the cancel function. You could put that in a wrapper function like you did |
|
Branch still fails with |
|
@jordanst3wart Good point, I'll update my fix and this should get rid of the linting messages as well. Thanks a lot! |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cli/commands/hcl/format/format.go(2 hunks)engine/engine.go(2 hunks)internal/cas/git.go(3 hunks)internal/os/exec/cmd.go(2 hunks)test/helpers/test_helpers.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- engine/engine.go
- test/helpers/test_helpers.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
Review the Go code for quality and correctness. Make sure that the Go code follows best practices, is performant, and is easy to understand and maintain.
Files:
internal/cas/git.gocli/commands/hcl/format/format.gointernal/os/exec/cmd.go
🧠 Learnings (4)
📚 Learning: 2025-07-03T22:05:07.356Z
Learnt from: wakeful
Repo: gruntwork-io/terragrunt PR: 4491
File: test/integration_test.go:4265-4265
Timestamp: 2025-07-03T22:05:07.356Z
Learning: Constants defined in one Go file are accessible from other files in the same package without explicit imports. When reviewing Go code, consider the package-level scope rather than just the individual file scope.
Applied to files:
internal/cas/git.go
📚 Learning: 2025-08-19T16:05:54.723Z
Learnt from: Resonance1584
Repo: gruntwork-io/terragrunt PR: 4683
File: go.mod:86-90
Timestamp: 2025-08-19T16:05:54.723Z
Learning: When analyzing Go module dependencies for removal, always check for both direct imports and API usage across all Go files in the repository, not just a quick search. The github.com/mattn/go-zglob library is used for filesystem walking and glob expansion in multiple Terragrunt files including util/file.go, format commands, and AWS provider patch functionality.
Applied to files:
cli/commands/hcl/format/format.gointernal/os/exec/cmd.go
📚 Learning: 2025-02-10T13:36:19.542Z
Learnt from: levkohimins
Repo: gruntwork-io/terragrunt PR: 3723
File: cli/commands/stack/action.go:160-160
Timestamp: 2025-02-10T13:36:19.542Z
Learning: The project uses a custom error package `github.com/gruntwork-io/terragrunt/internal/errors` which provides similar functionality to `fmt.Errorf` but includes stack traces. Prefer using this package's error functions (e.g., `errors.Errorf`, `errors.New`) over the standard library's error handling.
Applied to files:
cli/commands/hcl/format/format.gointernal/os/exec/cmd.go
📚 Learning: 2025-04-17T13:02:28.098Z
Learnt from: yhakbar
Repo: gruntwork-io/terragrunt PR: 4169
File: cli/commands/hcl/validate/cli.go:29-60
Timestamp: 2025-04-17T13:02:28.098Z
Learning: Avoid shadowing imported packages with local variables in Go code, such as using a variable named `flags` when the `github.com/gruntwork-io/terragrunt/cli/flags` package is imported. Use more specific variable names like `flagSet` instead.
Applied to files:
internal/os/exec/cmd.go
🧬 Code graph analysis (1)
internal/cas/git.go (1)
internal/cas/errors.go (1)
WrappedError(34-39)
🔇 Additional comments (1)
internal/cas/git.go (1)
266-266: Good variable rename.Renaming
contexttoerrContextavoids shadowing and improves clarity.Also applies to: 270-270
|
Apologies for the delay, I've been tied up the last days. I refactored the fix again and switched to overwriting the cancel function - the strict-lint runs now and the tests look good to. Please feel free to have another look |
5 participants
Description
Fixes #4839.
Replaces the current implementation with a helper implementation to avoid os.
TODOs
Read the Gruntwork contribution guidelines.
Release Notes (draft)
Migration Guide
No incompatibilities
Summary by CodeRabbit
New Features
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.