Skip to content

[conductor] Fix test race condition#13314

Merged
mslipper merged 4 commits intodevelopfrom
fix/conductor-test-race-condition
Dec 8, 2024
Merged

[conductor] Fix test race condition#13314
mslipper merged 4 commits intodevelopfrom
fix/conductor-test-race-condition

Conversation

@0x00101010
Copy link
Copy Markdown
Collaborator

@0x00101010 0x00101010 commented Dec 8, 2024

Description
Fix test race condition.

The race condition happens during shutdown phase instead of during initialization / startup phase.

The deadlock happens when prior tests tries to shutdown conductor, calls

if s.syncEnabled {
  s.wg.Add(1)
  s.next <- struct{}{}
}
s.NoError(s.conductor.Stop(s.ctx))
s.True(s.conductor.Stopped())

s.conductor.Stop(s.ctx) should've stopped the inner action for loop, and if it was stopped before the last step was executed, then the s.wg.Done() won't be called. However since we're reusing the same wg across tests before this, the next test hangs immediately since the wg starts from 1 instead of 0.

This fix creates a new wg for every test so that even if there's issue with prior test, new test would be able to continue working as it should

Tests
Re-enabled prior test

Additional context
Fixes #13276

Metadata

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 43.03%. Comparing base (4c88138) to head (bf19246).
Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #13314      +/-   ##
===========================================
- Coverage    47.03%   43.03%   -4.00%     
===========================================
  Files          927      758     -169     
  Lines        77983    68221    -9762     
  Branches       849        0     -849     
===========================================
- Hits         36682    29362    -7320     
+ Misses       38620    36383    -2237     
+ Partials      2681     2476     -205     
Flag Coverage Δ
cannon-go-tests-32 ?
cannon-go-tests-64 ?
contracts-bedrock-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
op-conductor/conductor/service.go 60.31% <100.00%> (+60.31%) ⬆️

... and 180 files with indirect coverage changes

@0x00101010 0x00101010 marked this pull request as ready for review December 8, 2024 19:38
@0x00101010 0x00101010 requested review from a team as code owners December 8, 2024 19:38
@0x00101010 0x00101010 requested a review from mslipper December 8, 2024 19:38
@mslipper mslipper added this pull request to the merge queue Dec 8, 2024
Merged via the queue into develop with commit 2824b3b Dec 8, 2024
@mslipper mslipper deleted the fix/conductor-test-race-condition branch December 8, 2024 20:25
sigma pushed a commit that referenced this pull request Dec 19, 2024
* Fix conductor test race condition

* update

* Address shell ci check

* rerun CI for non-related issue
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