Skip to content

Fix 2090 child workflow goroutine leak#2200

Merged
yuandrew merged 7 commits intotemporalio:masterfrom
kritibehl:fix-2090-child-workflow-goroutine-leak
Mar 2, 2026
Merged

Fix 2090 child workflow goroutine leak#2200
yuandrew merged 7 commits intotemporalio:masterfrom
kritibehl:fix-2090-child-workflow-goroutine-leak

Conversation

@kritibehl
Copy link
Contributor

@kritibehl kritibehl commented Feb 22, 2026

What was changed

Fix a goroutine leak in the test workflow environment when executing child workflows by registered type name.

Made doneChannel close idempotent using sync.Once.

Ensured doneChannel is closed from both:

startMainLoop() (deferred in the root environment)

Complete() (explicit close on workflow completion)

Consolidated leak regression coverage into a single test under test/ (using test/go.mod, which already includes go.uber.org/goleak).

Removed the previous internal/* leak tests and removed goleak from the root go.mod/go.sum.

Why?

When executing a child workflow by registered type name, a child testWorkflowEnvironmentImpl could remain blocked in startMainLoop() waiting on doneChannel.

In some completion paths, doneChannel was not guaranteed to be closed for the child environment, causing leaked goroutines.

This change ensures:

doneChannel is always closed exactly once

No race or double-close panic

Child workflow test environments exit cleanly

Regression coverage verifies no goroutines remain after workflow completion

Checklist

  1. Closes Starting a child workflow in tests leaks goroutines #2090
  2. How was this tested:
    go test ./... -count=1
    (cd test && go test -run TestChildWorkflowByName_NoGoroutineLeak -count=1 -v)

Verified no startMainLoop goroutine remains after workflow completion.

  1. Any docs updates needed?
    No.

@kritibehl kritibehl requested a review from a team as a code owner February 22, 2026 17:40
@CLAassistant
Copy link

CLAassistant commented Feb 22, 2026

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@kritibehl
Copy link
Contributor Author

Hi maintainers — this fork PR has “1 workflow awaiting approval”.
Could someone please approve and run the CI workflows?

Local verification:

  • go test ./... -count=1
  • go test -tags goleak ./internal -run TestChildWorkflowLeak_Goleak -count=1

Thanks!

@kritibehl
Copy link
Contributor Author

Pinned go.uber.org/goleak to v1.1.12 to match CI’s module resolution (oldstable/stable) and avoid go: updates to go.mod needed.
Verified locally:
go test ./... -count=1
go test -tags goleak ./internal -run TestChildWorkflowLeak_Goleak -count=1

@yuandrew
Copy link
Contributor

Thanks for the contribution! The fix makes sense to me. But I don't think we want to add goleak as a dependency to our root go.mod. Can we instead add a test to test/? test/go.mod already has the goleak dependency there. And do we need 2 separate tests? Our CI doesn't run tests with -tags goleak anyways, seems like we can consolidate into a single test here

@kritibehl
Copy link
Contributor Author

Thanks Andrew — totally agreed.

I’ll remove goleak from the root go.mod/go.sum and instead add the leak regression test under test/ (using test/go.mod where goleak already exists). Also agreed on consolidation: since CI doesn’t run -tags goleak, I’ll drop the split/tagged variant and keep a single test in test/ that reproduces the child-workflow-by-name path and verifies no goroutine leaks via goleak.

Next push will:

  • revert root module goleak changes (including the pin)
  • add one consolidated goleak-based regression test under test/
  • remove the redundant internal tests/build-tag version

@kritibehl
Copy link
Contributor Author

Changes have been pushed. Please let me know if anything else should be adjusted.

Copy link
Contributor

@yuandrew yuandrew left a comment

Choose a reason for hiding this comment

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

Fix and test looks good to me! Thanks again for the contribution 😃

@yuandrew yuandrew merged commit 9126a77 into temporalio:master Mar 2, 2026
13 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.

Starting a child workflow in tests leaks goroutines

3 participants