Skip to content

Conversation

shijiesheng
Copy link
Member

@shijiesheng shijiesheng commented Jun 2, 2025

What changed?

A pure revert of d9dcddc#diff-caac3c5868c1fd134487cacc53e459b7375722e23c6aba3e9407521feba00007

Why?

Nilness check is no longer enough after the commit

One alternative is to add additional emptiness check. This makes the client code error prone. Thus, a pure revert is desired.

How did you test it?

Unit Test

Potential risks

Copy link

codecov bot commented Jun 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.66%. Comparing base (50be4b8) to head (9279bdf).

Files with missing lines Coverage Δ
internal/internal_task_pollers.go 83.28% <100.00%> (ø)

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50be4b8...9279bdf. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.


activityTask := task.(*activityTask)
if activityTask.task == nil {
if activityTask.task == nil || len(activityTask.task.TaskToken) == 0 {
Copy link
Member

Choose a reason for hiding this comment

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

do we need a similar change on workflow tasks? since that path was changed too.

Copy link
Member Author

Choose a reason for hiding this comment

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

WorkflowTask doesn't need change since it's already handling emptiness here

Copy link
Member

Choose a reason for hiding this comment

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

👍 thanks, I'm convinced :) same kinds of paths as before then, should be safe.

@shijiesheng shijiesheng force-pushed the fix-panic-empty-poll branch from 9279bdf to a984be6 Compare June 2, 2025 23:55
@shijiesheng shijiesheng changed the title Fix panic on empty activity task Revert "Populate tasks in internal workflowTask and activityTask entities" Jun 2, 2025
@shijiesheng shijiesheng merged commit 6e22a27 into cadence-workflow:master Jun 3, 2025
9 of 10 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.

3 participants