Repository navigation
Conversation
Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Nikshay <nikshayyadav90@gmail.com>
✅ Deploy Preview for kptdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
🟡 Changes recommended
A Unix signal-completion race can swallow a termination signal instead of ending kpt.
1 open finding
What changed in this PR
This PR ensures timed-out exec functions terminate their Unix process groups, preventing child-process leaks.
Changes:
- Adds Unix process-group cleanup and signal forwarding.
- Preserves existing behavior on non-Unix platforms.
- Adds timeout and signal-forwarding tests.
| File | Description |
|---|---|
pkg/fn/runtime/exec.go |
Delegates command execution to the platform-specific helper. |
pkg/fn/runtime/exec_unix.go |
Implements process-group termination and signal forwarding. |
pkg/fn/runtime/exec_unix_test.go |
Tests child cleanup and signal forwarding. |
pkg/fn/runtime/exec_other.go |
Retains standard execution on non-Unix platforms. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Assisted-by: Claude:claude-opus-5-5 Signed-off-by: Nikshay <nikshayyadav90@gmail.com>
mozesl-nokia
left a comment
There was a problem hiding this comment.
I will be honest, I am not very keen on using os-specific code, but I doubt there is a much better way to do it.
Is there something we could do for Windows/DOS-based as well in a follow up? Even if we don't currently support Windows...
|
|
Thanks @mozesl-nokia! On Windows, terminating a Job Object on timeout should stop the child processes too. Opened #4820 for it :) |




Description
When an exec function times out, kpt now also stops the processes that the function started. Before, kpt killed only the function, and a process that the function had started (for example
sleepin a shell script) kept running after kpt had returned.On Linux and macOS (
//go:build unix):nohup, stays ignored.On other platforms nothing changes.
In
ExecFn.Runonly one line changes:cmd.Run()becomesrunCmd(ctx, cmd). The rest is in the new filesexec_unix.goandexec_other.go.Motivation
Fixes #4799. The issue came out of the review of #4798.
Notes for reviewers
These are the limits of this approach. I listed the first three in #4799 and asked there if they are acceptable.
/dev/tty, for example to ask for a passphrase, is now stopped with SIGTTIN, because its process group is not the foreground group of the terminal. It then waits until the timeout. I reproduced this under a pty. Fixing it means giving the terminal to the function the way a shell does, which is much more code.setsid, is still not stopped.kill <kpt pid>. SIGTERM that is sent to kpt alone used to leave the function running. It is now forwarded, so the function is stopped too.Testing
TestExecFn_TimeoutStopsChildProcesses: a function that waits for its child, and a function that has exited and left a child with its output open. In both cases the child is stopped when the timeout passes.TestExecFn_ForwardsSignal: a signal that kpt receives reaches the function, and is then sent to kpt itself.mainand of this branch, and sent the signals to the process group of kpt, as a terminal does:mainand this branchmainkill <kpt pid>go testforpkg/fn/runtime,thirdparty/kyaml/runfnandcommands/fn/..., the new tests with-race, andgo buildfor Linux and Windows all pass.AI usage
I have used AI in the creation of this PR.
I have used the following AI tools: