Skip to content

fix(core): avoid panic if workflow stream still up during eviction - #1158

Merged
chris-olszewski merged 1 commit into
masterfrom
olszewski/fix_flakey_ubuntu_arm
Mar 17, 2026
Merged

chris-olszewski merged 1 commit into
masterfrom
olszewski/fix_flakey_ubuntu_arm

Conversation

@chris-olszewski

Copy link
Copy Markdown
Member

What was changed

When the workflow stream has a zero-size cache (max_cached_workflows = 0) and ignore_evicts_on_shutdown = true (the default for tests), there is a race between stream shutdown and eviction completion processing.

After the final workflow activation completes, the zero-cache mechanism produces an eviction activation. When initiate_shutdown() is called, it cancels the shutdown token and enqueues a BumpStream message into the local channel. If an eviction completion is enqueued after BumpStream, the stream processes BumpStream first, sees shutdown_done() = true, and exits. This drops the eviction completion's response channel sender without fulfilling
it. This causes rx.await to return Err, which triggers dbg_panic! in tests.

For whatever reason, this only seemed to get hit on GitHub Action Ubuntu Arm runners.

Mirror the existing send_local-failure handling for empty completions for the case if the stream shuts down after WFActCompleteMsg was sent.

Why?

I've seen this test fail multiple times over the past week.

Checklist

  1. Closes N/A

  2. How was this tested:
    Added eviction_completion_during_shutdown_does_not_panic that reproduces the race by exploiting FIFO ordering on the local channel. It uses make_wft_stream_interminable() to prevent PollerDead from arriving, processes the full workflow, polls the eviction activation, then calls initiate_shutdown() followed immediately by the eviction completion. This guarentees BumpStream is ahead of WFActCompleteMsg in the channel. Without the fix the test panics with the same error seen in CI; with the fix it passes.

  3. Any docs updates needed?
    N/A

@chris-olszewski chris-olszewski left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Quick note

// The stream may shut down after accepting the message but before processing
// it (e.g. an eviction reply racing with shutdown_done()).
// Empty complete which is likely an evict reply, we can just ignore as above.
return Ok(());

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I didn't include a request_eviction call here. I can add it if that makes sense, but I think the workflow stream will always be shut down if the response send side of the channel has been dropped.

@chris-olszewski
chris-olszewski marked this pull request as ready for review March 16, 2026 22:48
@chris-olszewski
chris-olszewski requested a review from a team as a code owner March 16, 2026 22:48

@Sushisource Sushisource left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice, this fix makes sense. This one has been lurking forever and I never really found a satisfying way to fix it.

I kept trying to do this by ensuring that the other half of the channel never went away, which I do still think is sort of the "real" fix, but, at the same time if this was only ever happening with ignore_evicts_on_shutdown being set, then I really don't care and this is 100% fine.

@chris-olszewski
chris-olszewski force-pushed the olszewski/fix_flakey_ubuntu_arm branch from cd9aca4 to a592cab Compare March 16, 2026 23:49
@chris-olszewski
chris-olszewski merged commit 2f9d6c8 into master Mar 17, 2026
19 checks passed
@chris-olszewski
chris-olszewski deleted the olszewski/fix_flakey_ubuntu_arm branch March 17, 2026 12: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