refactor: isolate platform-specific process termination logic #259
Conversation
appleboy
commented
Jan 6, 2026
appleboy
commented
- Refactor platform-specific API key helper logic into separate files for Unix and Windows
- Remove direct process management and command execution code from the shared helper file
- Implement Unix-specific process group termination using signals for timeouts
- Implement Windows-specific Job Object termination for timeouts
- Improve code clarity and maintainability by isolating OS-specific behaviors
- Refactor platform-specific API key helper logic into separate files for Unix and Windows - Remove direct process management and command execution code from the shared helper file - Implement Unix-specific process group termination using signals for timeouts - Implement Windows-specific Job Object termination for timeouts - Improve code clarity and maintainability by isolating OS-specific behaviors Signed-off-by: Bo-Yi Wu <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR refactors platform-specific process termination logic by extracting the GetAPIKeyFromHelper implementation from the shared file into separate Unix and Windows-specific files. The Unix implementation uses process groups with SIGTERM/SIGKILL signals for timeout handling, while the Windows implementation uses Job Objects for reliable process tree termination.
Key changes:
- Split platform-specific process termination logic into
api_key_helper_unix.goandapi_key_helper_windows.go - Removed direct process execution code from the shared
api_key_helper.gofile - Implemented Unix-specific SIGTERM/SIGKILL two-phase termination for process groups
- Implemented Windows-specific Job Object termination with KILL_ON_JOB_CLOSE flag
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| util/api_key_helper.go | Removed platform-specific implementation and imports, retaining shared documentation and caching logic |
| util/api_key_helper_unix.go | New file implementing Unix/Linux/macOS process group termination using signals |
| util/api_key_helper_windows.go | New file implementing Windows Job Object-based process tree termination |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _ = cmd.Process.Kill() | ||
| _ = cmd.Wait() |
There was a problem hiding this comment.
The fallback error handling after failing to assign the process to the job object calls cmd.Wait() without any timeout protection. If the killed process doesn't clean up properly, this could block indefinitely. Consider using a timeout or goroutine pattern similar to the main execution path to prevent blocking during error recovery.
| // GetAPIKeyFromHelper executes a shell command to dynamically generate an API key. | ||
| // The command is executed in /bin/sh with a timeout controlled by the provided context. | ||
| // Platform-specific implementations are in api_key_helper_unix.go and api_key_helper_windows.go. | ||
| // | ||
| // The command is executed with a timeout controlled by the provided context. | ||
| // It returns the trimmed output from stdout, or an error if the command fails. | ||
| // | ||
| // On timeout, it kills the entire process group (shell and all descendants) using | ||
| // a two-phase approach: SIGTERM for graceful termination, then SIGKILL if needed. | ||
| // On timeout: | ||
| // - Unix/Linux/macOS: kills the entire process group (shell and all descendants) | ||
| // - Windows: terminates the Job Object (cmd.exe and all descendants) | ||
| // | ||
| // Security note: The returned API key is sensitive and should not be logged. |
There was a problem hiding this comment.
This documentation comment block for GetAPIKeyFromHelper is not attached to any function declaration in this file. The actual platform-specific implementations in api_key_helper_unix.go and api_key_helper_windows.go have their own complete documentation. Consider either removing this orphaned comment block or converting it to a package-level comment that explains the platform-specific implementations exist elsewhere.
| } | ||
| return apiKey, nil | ||
|
|
||
| case <-time.After(2 * time.Second): |
There was a problem hiding this comment.
The grace period duration of 2 seconds is hardcoded as a magic number. Consider extracting this to a named constant (e.g., 'gracePeriod' or 'terminationGracePeriod') to improve code maintainability and make it easier to adjust this value consistently if needed in the future.
- Assign the result of CloseHandle to underscore to avoid unchecked errors - Add PID validation to prevent integer overflow and invalid process IDs - Refactor resource cleanup to use deferred anonymous functions for handle closing Signed-off-by: Bo-Yi Wu <[email protected]>
- Improve timeout handling in GetAPIKeyFromHelper by ensuring no API key is returned after a timeout - Add explicit check for uninitialized process handle and report timeout error Signed-off-by: Bo-Yi Wu <[email protected]>
- Fix typo in error messages: change "timed out" to "timeout" for api_key_helper command errors Signed-off-by: Bo-Yi Wu <[email protected]>
- Add comprehensive Windows-specific tests for API key helper functionality - Test job object creation and validation of process termination on job close - Verify error handling for invalid and non-existent process IDs - Ensure API key extraction works for various Windows command scenarios - Test timeout behavior and process tree termination using job objects - Check error handling for failed commands and empty output cases - Validate that sensitive stderr output is not leaked in error messages - Confirm correct handling of context cancellation and concurrent invocations Signed-off-by: Bo-Yi Wu <[email protected]>
