Skip to content

implement err group as an alternative to a wait group so errors from activities can be propagated upwards nicely - #462

Merged
cschleiden merged 4 commits into
cschleiden:mainfrom
DerkSchooltink:feature/implement-err-group
May 29, 2026
Merged

implement err group as an alternative to a wait group so errors from activities can be propagated upwards nicely#462
cschleiden merged 4 commits into
cschleiden:mainfrom
DerkSchooltink:feature/implement-err-group

Conversation

@DerkSchooltink

Copy link
Copy Markdown
Contributor

implement #461

…activities can be propagated upwards nicely
@DerkSchooltink
DerkSchooltink marked this pull request as ready for review December 1, 2025 10:36
@cschleiden
cschleiden requested a review from Copilot February 28, 2026 15:03

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a workflow-native ErrGroup (errgroup-style) abstraction so concurrent workflow coroutines can return errors that are aggregated and propagated via Wait, and includes a sample demonstrating the new API.

Changes:

  • Expose workflow.ErrGroup and workflow.WithErrGroup as public workflow APIs.
  • Implement internal/sync.ErrGroup with tests for success and “first error wins”.
  • Add a new sample (samples/concurrent-errgroup) demonstrating concurrent activity execution with error propagation.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
workflow/sync.go Exposes the new ErrGroup type alias and WithErrGroup helper at the workflow API layer.
internal/sync/errgroup.go Implements the scheduler-compatible errgroup abstraction.
internal/sync/errgroup_test.go Adds unit tests validating basic ErrGroup behavior.
samples/concurrent-errgroup/concurrent_errgroup.go Demonstrates usage of workflow.WithErrGroup in a runnable sample.
Comments suppressed due to low confidence (2)

internal/sync/errgroup.go:40

  • errGroup has a waiting flag intended for misuse detection, but it’s currently unused beyond being set in Wait. Either remove it to avoid dead state, or use it to enforce a clear contract (e.g., disallow calling Go after Wait starts, similar to WaitGroup).

This issue also appears on line 58 of the same file.

	// track if Wait was called to detect certain misuses (optional)
	waiting bool

internal/sync/errgroup.go:60

  • Wait sets waiting=true, but Go does not check it. This allows Go to be called after Wait has started/returned, which can make Wait return without waiting for the newly launched coroutine (i.e., silent misuse). Consider adding a panic/guard in Go when waiting is already true (mirroring WaitGroup’s misuse detection) so incorrect usage fails fast.
func (g *errGroup) Go(f func(Context) error) {
	g.n += 1


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread internal/sync/errgroup.go Outdated
Comment thread internal/sync/errgroup.go
Future.Get does not observe ctx.Done, so the doc comment promising
cancellation-aware Wait was incorrect. Aligns with WaitGroup behavior.
Also wires up the waiting flag to panic on Go called after Wait, matching
the WaitGroup.Add misuse detection pattern.
@DerkSchooltink
DerkSchooltink force-pushed the feature/implement-err-group branch from b0a3bae to 8968430 Compare April 29, 2026 20:24
@cschleiden
cschleiden enabled auto-merge (rebase) May 25, 2026 03:13
@cschleiden
cschleiden merged commit 6298a19 into cschleiden:main May 29, 2026
5 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