Skip to content

Conversation

@Shaddoll
Copy link
Member

@Shaddoll Shaddoll commented Sep 2, 2025

What changed?
This is a fix for #7161

Why?
The original PR was to split a virtual queue into virtual slices to prevent a virtual slice from growing too large. However, the MergeSlices method nullify the effect of AppendSlices method because that method merges all the existing virtual slices in the virtual queue with the incoming virtual slices, which is not expected when we add new tasks to the virtual queue.

How did you test it?
unit tests & integration tests

Potential risks

history queue v2 might be broken and workflows can get stuck

Release notes

Documentation Changes

Copy link
Contributor

@c-warren c-warren left a comment

Choose a reason for hiding this comment

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

General comments:

We're using x := blah.(VirtualSlice) a lot wherever we're interacting with the underlying VirtualSlice and accessing elements. Though we should only have VirtualSlice elements, this syntax can panic at runtime if anyone introduces a bug down the line. I'd recommend protecting against it with either x, ok := blah.(VirtualSlice) so we can catch it earlier, or explicitly documenting that these methods panic if something goes wrong, or (the most effort) moving away from using container/list and writing our own double linked list that supports generics - so that we can guarantee that the type is always what we expect it to be, and catch any errors at compile time instead.

I'd recommend updating the uses of slices list.List (e.g in appendOrMergeSlice) to use the virtualSlice terminology, to reduce confusion between our construct and the core go type.

@Shaddoll Shaddoll enabled auto-merge (squash) September 3, 2025 22:10
@Shaddoll Shaddoll merged commit fbfbeb1 into cadence-workflow:master Sep 3, 2025
30 checks passed
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