Skip to content

Conversation

maxdml
Copy link
Collaborator

@maxdml maxdml commented Aug 16, 2025

Our current method for capturing the reflection name of a step is flawed. The typed erase function (anonymous) always has the same name, and thus every call to RunAsStep overwrites it. This of course results in wrong data in the operation_results table.

There is no good way to associated a unique key that the interface method can use on this map. Hence, let's just move to the cleaner functional options interface to handle step options, voiding the need for the in-memory map.

Added a test (TestSteps/checkStepName) that fails without this change.

@maxdml maxdml marked this pull request as ready for review August 16, 2025 00:38
Copy link
Member

@kraftp kraftp left a comment

Choose a reason for hiding this comment

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

Good fix, but why did no existing test catch this bug?

Copy link
Member

@kraftp kraftp left a comment

Choose a reason for hiding this comment

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

Also, could anything else have a similar problem? What about workflows?

@maxdml
Copy link
Collaborator Author

maxdml commented Aug 16, 2025

Good fix, but why did no existing test catch this bug?

Because we never had a tests that checked the step names after consecutive workflow / run a step invocations.

That being said I wasn't able to reproduce the exact bug the widget store showed. The problem became quickly obvious when debugging, so I moved on to the fix.

could anything else have a similar problem? What about workflows?

Thank you for raising the question, there is actually an additional bug where a user could register two different workflows under the same custom name. Which I fixed in 6f8ced2.

@maxdml maxdml merged commit a680f8f into main Aug 16, 2025
2 of 4 checks passed
@maxdml maxdml deleted the steps-options branch August 16, 2025 01:24
@kraftp
Copy link
Member

kraftp commented Aug 16, 2025

Good fix, but why did no existing test catch this bug?

Because we never had a tests that checked the step names after consecutive workflow / run a step invocations.

That being said I wasn't able to reproduce the exact bug the widget store showed. The problem became quickly obvious when debugging, so I moved on to the fix.

could anything else have a similar problem? What about workflows?

Thank you for raising the question, there is actually an additional bug where a user could register two different workflows under the same custom name. Which I fixed in 6f8ced2.

That doesn't answer the question. The bug I observed wasn't a failure to write step names, it was a failure in recovery. Why did our recovery tests not catch it?

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