Skip to content

fix: process pool concurrency#1358

Merged
krishankumar01 merged 5 commits intomasterfrom
kkumar-gcc/optimise-process
Jan 25, 2026
Merged

fix: process pool concurrency#1358
krishankumar01 merged 5 commits intomasterfrom
kkumar-gcc/optimise-process

Conversation

@krishankumar01
Copy link
Member

@krishankumar01 krishankumar01 commented Jan 24, 2026

📑 Description

image image

✅ Checks

  • Added test cases for my code

@krishankumar01 krishankumar01 requested a review from a team as a code owner January 24, 2026 12:25
@codecov
Copy link

codecov bot commented Jan 24, 2026

Codecov Report

❌ Patch coverage is 90.74074% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.64%. Comparing base (632170c) to head (976dce2).
⚠️ Report is 5 commits behind head on master.

Files with missing lines Patch % Lines
process/pool.go 77.27% 4 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1358      +/-   ##
==========================================
+ Coverage   70.60%   70.64%   +0.03%     
==========================================
  Files         286      286              
  Lines       17612    17665      +53     
==========================================
+ Hits        12435    12479      +44     
- Misses       4652     4658       +6     
- Partials      525      528       +3     

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

@krishankumar01
Copy link
Member Author

@CodeRabbit review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 24, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 24, 2026

📝 Walkthrough

Walkthrough

The changes refactor the process pool's concurrency model from per-job goroutine waiting to a collector pattern with a RunningPool abstraction, add thread-safety through mutexes in tests, introduce a new deprecated GetJson method to the Application type, and disable the Telemetry binding entry.

Changes

Cohort / File(s) Summary
Binding Configuration
contracts/binding/binding.go
Commented out Telemetry entry from exported Bindings map with a TODO note, effectively disabling it without removing other entries.
Foundation API Extensions
foundation/application.go
Added new public method GetJson() returning foundation.Json, marked as deprecated with guidance to use Json() instead.
Process Pool Concurrency Refactoring
process/pool.go, process/running_pool.go
Replaced per-job goroutine waiting with collector pattern and RunningPool abstraction. Pool workers now execute synchronously, pushing results to shared channel consumed by dedicated collector. RunningPool redesigned with mutex-protected maps for processes and results, new setProcess and setResult helpers, and simplified NewRunningPool signature.
Process Pool Unix Tests
process/pool_unix_test.go, process/running_pool_unix_test.go
Added mutex synchronization in test callbacks for thread safety. Updated NewRunningPool call sites to match simplified constructor signature (6 parameters vs. 8, removed results map and placeholder nils).
Process Pool Windows Tests
process/pool_windows_test.go, process/running_pool_windows_test.go
Added mutex protection around shared state in OnOutput callbacks. Introduced long-running PowerShell command (Start-Sleep -Seconds 10) in signal handling test with updated assertions for expected failure state.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~65 minutes

Poem

🐰 The pool now flows with ordered grace,
No goroutines race through this space,
A collector gathers results with care,
While mutexes guard our data fair,
RunningPool orchestrates the way,
Synchronized threads in perfect ballet! 🎭

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'fix: process pool concurrency' accurately reflects the primary changes in the pull request, which focus on reworking the process pool's concurrency model and synchronization mechanisms.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing touches
  • 📝 Generate docstrings

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.

hwbrzzl
hwbrzzl previously approved these changes Jan 24, 2026
Copy link
Contributor

@hwbrzzl hwbrzzl left a comment

Choose a reason for hiding this comment

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

LGTM, could you add some screenshots for the local test?

@krishankumar01
Copy link
Member Author

@hwbrzzl done

Copy link
Contributor

@hwbrzzl hwbrzzl left a comment

Choose a reason for hiding this comment

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

Awesome

@krishankumar01 krishankumar01 merged commit a68fb3f into master Jan 25, 2026
18 checks passed
@krishankumar01 krishankumar01 deleted the kkumar-gcc/optimise-process branch January 25, 2026 07:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants