Skip to content

feat(windows): implement multi platform process kill#1875

Merged
brandonskiser merged 4 commits intoaws:mainfrom
cwoolum:main
May 19, 2025
Merged

feat(windows): implement multi platform process kill#1875
brandonskiser merged 4 commits intoaws:mainfrom
cwoolum:main

Conversation

@cwoolum
Copy link
Contributor

@cwoolum cwoolum commented May 17, 2025

Summary

This PR enhances the Amazon Q CLI with cross-platform process termination capabilities and optimizes Windows performance through stack size adjustments. These improvements ensure consistent behavior across operating systems while addressing Windows-specific memory constraints.

Changes

• Added Windows-specific implementation for process termination
• Ensured consistent behavior between Windows, macOS, and Linux platforms
• Implemented proper error handling for process termination failures
Increased thread stack size allocation for Windows environments
• Optimized memory management for Windows-specific constraints

Technical Details

Process Termination

• Implemented platform-specific code paths for process termination
• Used Windows API for Windows and POSIX signals for Unix-based systems
• Added proper error handling and logging for failed termination attempts

Stack Size Optimization

• Increased default thread stack size on Windows to prevent stack overflow errors
• Windows has different memory management characteristics compared to Unix-based systems, requiring larger stack allocations
• This change prevents crashes during deep recursion or when handling complex data structures
• The stack size increase is crucial for stable operation on Windows, particularly when processing large amounts of data or during intensive operations

Default Stack Size Differences

Windows default stack size: 1MB for user threads (significantly smaller than Linux)
Linux default stack size: 8MB for user threads
• This 8x difference is critical when running memory-intensive operations
• Windows' smaller default stack size can lead to stack overflow errors in scenarios that work fine on Linux
• Our increase aligns the Windows stack size more closely with Linux defaults, ensuring consistent behavior across platforms
• This adjustment is particularly important for recursive operations and deep call stacks that are common in our CLI operations

Testing

• Verified process termination works correctly on Windows systems
• Confirmed backward compatibility with existing Unix/Linux implementations

@codecov-commenter
Copy link

codecov-commenter commented May 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 16.75%. Comparing base (79059f4) to head (87061f3).
Report is 21 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1875   +/-   ##
=======================================
  Coverage   16.75%   16.75%           
=======================================
  Files         213      213           
  Lines       20704    20704           
  Branches      871      871           
=======================================
  Hits         3468     3468           
  Misses      17236    17236           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cwoolum cwoolum force-pushed the main branch 5 times, most recently from 70dc97c to 2779ab1 Compare May 19, 2025 20:08
@cwoolum cwoolum marked this pull request as ready for review May 19, 2025 20:34
brandonskiser
brandonskiser previously approved these changes May 19, 2025

// Helper to create a long-running process for testing
fn spawn_test_process() -> std::process::Child {
let mut command = Command::new("cmd");
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this command doing? Would rather tests not do stuff like send network requests to localhost if we can rather do a sleep 10000 or something otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call, no need to make network calls. Changed to be a timeout instead


use crate::platform::Env;

/// Fields for OS release information
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we include the reference to the Linux manpage still in this doc comment? https://www.man7.org/linux/man-pages/man5/os-release.5.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@brandonskiser brandonskiser merged commit 25cdf07 into aws:main May 19, 2025
11 checks passed
This was referenced May 21, 2025
hayemaxi pushed a commit to hayemaxi/amazon-q-developer-cli that referenced this pull request May 22, 2025
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.

3 participants