Skip to content

Remove raciness from test_is_process_on_port_concurrent_access#5826

Merged
adhami3310 merged 1 commit intomainfrom
masenf/test_is_process_on_port_concurrent_access_flake
Sep 27, 2025
Merged

Remove raciness from test_is_process_on_port_concurrent_access#5826
adhami3310 merged 1 commit intomainfrom
masenf/test_is_process_on_port_concurrent_access_flake

Conversation

@masenf
Copy link
Collaborator

@masenf masenf commented Sep 25, 2025

This test was consistently failing on macos github actions runners.

This test was consistently failing on macos github actions runners.
Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Greptile Overview

Summary

This PR fixes a race condition in the test_is_process_on_port_concurrent_access test that was causing failures on macOS GitHub Actions runners. The fix replaces unreliable time.sleep() calls with proper thread synchronization using threading.Event objects and robust polling with AppHarness._poll_for().

  • Replaced time.sleep() timing dependencies with threading.Event synchronization
  • Added proper timeout handling using DEFAULT_TIMEOUT constant
  • Used AppHarness._poll_for() for reliable polling instead of fixed delays
  • Added proper cleanup with try/finally blocks and timeout-aware thread joining

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • Score reflects a well-implemented race condition fix that follows best practices for thread synchronization, uses existing testing utilities properly, and includes proper timeout handling and cleanup
  • No files require special attention

Important Files Changed

File Analysis

Filename        Score        Overview
tests/units/utils/test_processes.py 5/5 Race condition fixed by replacing time.sleep() with proper threading.Event synchronization and polling

Sequence Diagram

sequenceDiagram
    participant MT as Main Thread
    participant ST as Server Thread
    participant S as Socket Server
    participant EO as is_open Event
    participant EC as do_close Event
    participant AH as AppHarness._poll_for

    MT->>ST: Start thread
    ST->>S: Create socket
    ST->>S: Bind to port
    ST->>S: Listen on port
    ST->>EO: Set is_open event
    MT->>EO: Wait for is_open (with timeout)
    EO-->>MT: Server is ready
    
    MT->>AH: Poll for port occupation
    AH->>AH: Check is_process_on_port(shared)
    AH-->>MT: Port is occupied (success)
    
    MT->>EC: Set do_close event
    ST->>EC: Wait for do_close (with timeout)
    EC-->>ST: Close signal received
    ST->>S: Close socket
    
    MT->>AH: Poll for port release  
    AH->>AH: Check not is_process_on_port(shared)
    AH-->>MT: Port is released (success)
    
    MT->>ST: Join thread (with timeout)
Loading

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@codspeed-hq
Copy link

codspeed-hq bot commented Sep 25, 2025

CodSpeed Performance Report

Merging #5826 will not alter performance

Comparing masenf/test_is_process_on_port_concurrent_access_flake (b053078) with main (269723b)

Summary

✅ 8 untouched

@adhami3310 adhami3310 merged commit e2105fe into main Sep 27, 2025
51 of 53 checks passed
@adhami3310 adhami3310 deleted the masenf/test_is_process_on_port_concurrent_access_flake branch September 27, 2025 01:11
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.

2 participants