Skip to content

Fix race condition in retryProcess causing TestShutdown_DrainsRetriedTasks to fail - #5112

Closed
Future-Outlier with Copilot wants to merge 2 commits into
masterfrom
copilot/fix-build-historyserver-job
Closed

Fix race condition in retryProcess causing TestShutdown_DrainsRetriedTasks to fail#5112
Future-Outlier with Copilot wants to merge 2 commits into
masterfrom
copilot/fix-build-historyserver-job

Conversation

Copilot AI commented Aug 9, 2026

Copy link
Copy Markdown

TestShutdown_DrainsRetriedTasks was intermittently failing because retryProcess had an early-exit guard that raced with shutdown() closing stopProducers.

The race: mockWriter.WriteFile sets failNext=false before returning the error. The test's require.Eventually fires on that signal, immediately calling ec.shutdown(). But retryProcess is called a few instructions later inside processRotatedFile — after WriteFile returns but before the goroutine is spawned. If stopProducers is already closed when the guard runs, the retry is silently abandoned.

Fix: Replace the entry-point guard in retryProcess with a finalAttempt flag on rotationTask:

  • Remove the select { case <-ec.stopProducers: return } guard at the top of retryProcess — the goroutine is now always spawned.
  • When the goroutine is woken by stopProducers (shutdown interrupt), set task.finalAttempt = true.
  • In processRotatedFile, if upload fails and task.finalAttempt is true, log and abandon rather than re-entering retryProcess — this preserves the single-final-attempt contract and prevents infinite retry loops.

Why are these changes needed?

TestShutdown_DrainsRetriedTasks in historyserver/pkg/collector/eventcollector was failing in CI due to a race condition: the old guard in retryProcess could fire before the retry goroutine was spawned, silently dropping the task instead of making one final upload attempt during shutdown.

Related issue number

Labels

  • If this PR has user-facing changes that require documentation updates at release time, I have added the doc-updates-required label.
  • If this PR contains breaking changes, I have added the breaking-change label.

Checks

  • I've made sure the tests are passing.
  • Testing Strategy
    • Unit tests
    • Manual tests
    • This PR is not tested :(

…Tasks to fail

Co-authored-by: Future-Outlier <76461262+Future-Outlier@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix failing GitHub Actions job Build historyserver Fix race condition in retryProcess causing TestShutdown_DrainsRetriedTasks to fail Aug 9, 2026
Copilot AI requested a review from Future-Outlier August 9, 2026 15:45
@machichima

Copy link
Copy Markdown
Collaborator

There's an existing PR here: #5105

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.

3 participants