Skip to content

fix: add platform-specific functions for process management syscall#1541

Merged
chengfang merged 1 commit intoargoproj-labs:masterfrom
chengfang:platform.specific.syscall
Mar 16, 2026
Merged

fix: add platform-specific functions for process management syscall#1541
chengfang merged 1 commit intoargoproj-labs:masterfrom
chengfang:platform.specific.syscall

Conversation

@chengfang
Copy link
Collaborator

@chengfang chengfang commented Mar 16, 2026

Fixes #1512

Summary by CodeRabbit

  • Refactor
    • Improved internal process lifecycle management for git-related operations with optimized, platform-specific behavior for Unix and Windows systems.

Signed-off-by: Cheng Fang <cfang@redhat.com>
Co-authored-by: Claude <noreply@anthropic.com>
@coderabbitai
Copy link

coderabbitai bot commented Mar 16, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 0ffae046-33fa-44e5-9157-b2e8c10b5b9a

📥 Commits

Reviewing files that changed from the base of the PR and between ad20e7e and fc7ba7d.

📒 Files selected for processing (3)
  • ext/git/client.go
  • ext/git/procattr_unix.go
  • ext/git/procattr_windows.go

Walkthrough

Introduces platform-specific abstractions for process group management in Git subprocess execution. Refactors client.go to use three new helper functions instead of direct syscall operations, with Unix and Windows implementations handling different timeout behaviors and process termination strategies.

Changes

Cohort / File(s) Summary
Client refactoring
ext/git/client.go
Replaces direct syscall usage (Setpgid configuration and Kill operations) with helper function calls: setSysProcAttr(), killProcessGroup(), and newTimeoutBehavior(). Removes syscall import and streamlines process management logic.
Unix-specific implementation
ext/git/procattr_unix.go
Adds Unix-specific helpers that configure process groups (Setpgid), terminate process groups with SIGKILL, and set timeout behavior to use SIGTERM with ShouldWait enabled for proper subprocess cleanup on cancellation.
Windows-specific implementation
ext/git/procattr_windows.go
Adds Windows-specific helpers as no-ops for process group operations (since Windows doesn't support process groups), and configures timeout behavior to use SIGKILL without waiting, adapted for Windows platform constraints.

Possibly related PRs

  • #1477: Related modifications to git subprocess handling that ensure proper cleanup of child processes through structured process management helpers.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the main change: introducing platform-specific helper functions to manage process syscalls across Windows and Unix platforms.
Linked Issues check ✅ Passed The PR implementation fully addresses all coding requirements from issue #1512: adds platform-specific functions (setSysProcAttr, killProcessGroup, newTimeoutBehavior) with proper !windows and windows build tags.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the Windows build failure by refactoring syscall usage into platform-specific helpers; no unrelated modifications are present.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

📝 Coding Plan
  • Generate coding plan for human review comments

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Tip

CodeRabbit can enforce grammar and style rules using `languagetool`.

Configure the reviews.tools.languagetool setting to enable/disable rules and categories. Refer to the LanguageTool Community to learn more.

@codecov-commenter
Copy link

codecov-commenter commented Mar 16, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 73.62%. Comparing base (c1674be) to head (fc7ba7d).
⚠️ Report is 61 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1541      +/-   ##
==========================================
+ Coverage   71.48%   73.62%   +2.13%     
==========================================
  Files          50       53       +3     
  Lines        4667     5110     +443     
==========================================
+ Hits         3336     3762     +426     
- Misses       1133     1142       +9     
- Partials      198      206       +8     

☔ 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.

@chengfang chengfang merged commit 5a74980 into argoproj-labs:master Mar 16, 2026
12 checks passed
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.

Failed to build windows binary due to platform-dependent syscall

3 participants