ThreadPool concurrency refactoring (#2220)

[MitchLewis930]

PR_053

[greptile-apps]

Greptile Overview

Greptile Summary

This PR refactors the ThreadPool implementation to fix concurrency bugs affecting non-GVL Ruby implementations (JRuby, TruffleRuby).

Key Changes:

• Added with_mutex helper to prevent mutex re-locking when already owned, addressing potential deadlocks
• Moved worker thread cleanup logic inside the mutex-synchronized block during trim operations, fixing race conditions where @spawned and @workers could become inconsistent
• Added synchronization during initialization to ensure worker threads reach their wait state before the pool is considered ready
• Unified shutdown logic to use the existing trim mechanism by setting @trim_requested = @spawned
• Completely rewrote tests using a MutexPool wrapper to eliminate flaky race conditions and sleep-based timing

Impact:
The changes improve thread safety for non-GVL Rubies where true parallelism can expose race conditions that are masked by the GVL in MRI. Tests are now deterministic and reliable across all Ruby implementations.

Confidence Score: 4/5

• This PR is safe to merge with minor style considerations
• The refactoring improves concurrency safety with well-tested changes. Confidence is 4/5 due to a minor style suggestion for the ternary operator formatting in with_mutex. The core logic is sound, tests are comprehensive, and the changes address real concurrency issues.
• No files require special attention - all changes follow established patterns and are well-tested

Important Files Changed

Filename	Overview
History.md	Documentation update describing concurrency bug fixes and refactoring
lib/puma/thread_pool.rb	Refactored thread pool with with_mutex helper, improved thread cleanup, and initialization synchronization
test/test_thread_pool.rb	Added MutexPool test helper and refactored tests to eliminate race conditions and timeouts

Sequence Diagram

sequenceDiagram
    participant Main as Main Thread
    participant TP as ThreadPool
    participant WT as Worker Thread
    participant Mutex as @mutex
    
    Note over Main,TP: Initialization (lines 56-61)
    Main->>+TP: initialize(min, max)
    loop min times
        TP->>Mutex: synchronize
        TP->>+WT: spawn_thread
        WT->>WT: Setup local vars
        WT->>Mutex: synchronize
        WT->>WT: @waiting += 1
        WT->>Main: signal @not_full
        WT->>WT: wait @not_empty
        Note over Main,WT: Wait ensures worker is ready
        Main->>Main: @not_full.wait(@mutex)
        Main->>TP: Continue initialization
    end
    
    Note over Main,WT: Adding Work (<<)
    Main->>+TP: << (work)
    TP->>Mutex: with_mutex
    alt mutex already owned
        TP->>TP: yield (no re-lock)
    else mutex not owned
        TP->>Mutex: synchronize
    end
    TP->>TP: @todo << work
    TP->>WT: signal @not_empty
    WT->>-WT: Wake up, grab work
    
    Note over TP,WT: Trim Request (lines 107-111)
    Main->>TP: trim
    TP->>Mutex: with_mutex
    TP->>TP: @trim_requested += 1
    TP->>WT: signal @not_empty
    WT->>Mutex: synchronize
    WT->>WT: Check @trim_requested > 0
    WT->>WT: @trim_requested -= 1
    WT->>WT: @spawned -= 1
    WT->>WT: @workers.delete(self)
    WT->>WT: Thread.exit
    Note over WT: Cleanup moved inside mutex
    
    Note over Main,WT: Shutdown (line 284)
    Main->>TP: shutdown
    TP->>Mutex: with_mutex
    TP->>TP: @shutdown = true
    TP->>TP: @trim_requested = @spawned
    TP->>WT: broadcast @not_empty
    WT->>WT: Exit via trim logic

Loading

[greptile-apps]

_{3 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Ternary operator spans lines without clear continuation. Consider reformatting for clarity:

Suggested change

	@mutex.owned? ?
	yield :
	@mutex.owned? ? yield : @mutex.synchronize(&block)

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

Prompt To Fix With AI

This is a comment left during a code review.
Path: lib/puma/thread_pool.rb
Line: 143:144

Comment:
Ternary operator spans lines without clear continuation. Consider reformatting for clarity:

```suggestion
    @mutex.owned? ? yield : @mutex.synchronize(&block)
```

<sub>Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!</sub>

How can I resolve this? If you propose a fix, please make it concise.