Skip to content

fix(worker): replace time.Sleep with channel sync in consumer manager test#7971

Open
arzonus wants to merge 1 commit intocadence-workflow:masterfrom
arzonus:fix-flaky-consumer-manager-enabled-disabled
Open

fix(worker): replace time.Sleep with channel sync in consumer manager test#7971
arzonus wants to merge 1 commit intocadence-workflow:masterfrom
arzonus:fix-flaky-consumer-manager-enabled-disabled

Conversation

@arzonus
Copy link
Copy Markdown
Contributor

@arzonus arzonus commented Apr 17, 2026

What changed?

  • Added unexported afterIterFn func() field and withAfterIterFn option to ConsumerManager (service/worker/asyncworkflow/async_workflow_consumer_manager.go). The run() loop calls the hook after each ticker iteration (nil-guarded; zero overhead in production).
  • Updated TestConsumerManagerEnabledDisabled to use the hook instead of time.Sleep(50ms). Added mockTimeSrc.BlockUntil(1) before the first Advance to ensure the ticker is registered before advancing time.

Why?
time.Sleep(50ms) is not a reliable synchronization primitive — under CI load the background goroutine may not finish processing the tick in time. The hook signals completion precisely. BlockUntil(1) prevents the race where Advance fires before the run() goroutine has called NewTicker, which would leave the advance with no timer to fire and cause the test to deadlock.

Detected as a flaky test: TestConsumerManagerEnabledDisabled fired 3× in CI (Jan–Apr 2026).

How did you test it?

go test -race -count=5 -timeout 30s \
  -run TestConsumerManagerEnabledDisabled \
  ./service/worker/asyncworkflow/...

All pass.

Potential risks
Low. afterIterFn is unexported, nil-guarded (zero overhead in production), called only within the ticker.Chan() select arm. The production ConsumerManager always has afterIterFn == nil.

Release notes
N/A

Documentation Changes
N/A

… test

time.Sleep(50ms) after Advance() is unreliable under CI load.
Add an afterIterFn test hook called after each ticker iteration so the
test can synchronize precisely. Use BlockUntil(1) before the first
Advance to ensure the run() goroutine has created the ticker.

Fixes flaky TestConsumerManagerEnabledDisabled (3x in CI Jan-Apr 2026).

Signed-off-by: Seva Kaloshin <seva.kaloshin@gmail.com>
@gitar-bot
Copy link
Copy Markdown

gitar-bot Bot commented Apr 17, 2026

Code Review ✅ Approved

Replaced time.Sleep with channel synchronization in the consumer manager test to improve reliability and execution speed. No issues found.

Rules ✅ All requirements met

Repository Rules

PR Description Quality Standards: The PR description follows the required structure with clear technical rationale, testing commands, and risk assessment.

2 rules not applicable. Show all rules by commenting gitar display:verbose.

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply Compact
gitar auto-apply:on         
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Gitar

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