Skip to content

Error-handling cleanup: surface swallowed failures, branch on values not strings #411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lilydoar wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Error-handling cleanup: surface swallowed failures, branch on values not strings #411

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4abecc8
Return dropped search-attribute registration error in ebb_and_flow
lilydoar Jun 25, 2026
3275fe0
Surface failed ebbAndFlowTrack workflows instead of swallowing them
lilydoar Jun 25, 2026
faa9609
Fix dropped terminate error and pointer log in ExecuteKitchenSinkWork…
lilydoar Jun 25, 2026
4e7ea36
Stop double-handling terminal iteration errors in generic_executor
lilydoar Jun 25, 2026
3f265c3
Branch on gRPC status code, not error string, in RegisterDefaultSearc…
lilydoar Jun 25, 2026
e940ea4
Wrap cert-load error with %w in loadTLSConfig
lilydoar Jun 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion clioptions/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ func (c *ClientOptions) loadTLSConfig() (*tls.Config, error) {
}
cert, err := tls.LoadX509KeyPair(c.ClientCertPath, c.ClientKeyPath)
if err != nil {
return nil, fmt.Errorf("failed to load certs: %s", err)
return nil, fmt.Errorf("failed to load certs: %w", err)
}
tlsConfig.Certificates = []tls.Certificate{cert}
return tlsConfig, nil
Expand Down
5 changes: 4 additions & 1 deletion loadgen/generic_executor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,11 +151,14 @@ func (g *genericRun) Run(ctx context.Context) error {

backoff, retry := run.ShouldRetry(err)
if retry {
// Transient: this attempt is superseded by the retry and
// never propagated, so log it here as the only record.
err = fmt.Errorf("iteration %v encountered error: %w", run.Iteration, err)
g.logger.Error(err)
} else {
// Terminal: propagated via doneCh and reported by Run's
// caller, so don't also log it here.
err = fmt.Errorf("iteration %v failed: %w", run.Iteration, err)
g.logger.Error(err)
break retryLoop
}

Expand Down
29 changes: 8 additions & 21 deletions loadgen/scenario.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,22 +306,10 @@ func (s *ScenarioInfo) RegisterDefaultSearchAttributes(ctx context.Context) erro
},
Namespace: s.Namespace,
})
// Throw an error if the attributes could not be registered, but ignore already exists errs
alreadyExistsStrings := []string{
"already exists",
"attributes mapping unavailble",
}
if err != nil {
isAlreadyExistsErr := false
for _, s := range alreadyExistsStrings {
if strings.Contains(err.Error(), s) {
isAlreadyExistsErr = true
break
}
}
if !isAlreadyExistsErr {
return fmt.Errorf("failed to register search attributes: %w", err)
}
// Throw an error if the attributes could not be registered, but ignore the
// case where they already exist (re-registration on a reused namespace).
if err != nil && status.Code(err) != codes.AlreadyExists {
return fmt.Errorf("failed to register search attributes: %w", err)
Comment on lines +309 to +312

@THardy98 THardy98 Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I had assumed that there maybe were cases (i.e. old server versions) where we don't return codes.AlreadyExists, hence the string matching.

@stephanos I think you wrote this? Are we ok to do away with the string matching now?

@lilydoar lilydoar Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

One thing worth flagging regardless of the codes.AlreadyExists decision: the second string in the old list, "attributes mapping unavailble", is a typo — the real server message is ...unavailable. So strings.Contains never matched it, meaning only the "already exists" branch was ever actually doing anything. Removing that second string drops no working behavior.

}
return nil
}
Expand Down Expand Up @@ -428,14 +416,13 @@ func (r *Run) ExecuteKitchenSinkWorkflow(ctx context.Context, options *KitchenSi
err := executor.ExecuteClientSequence(cancelCtx, clientSeq)
if err != nil {
clientActionsErrPtr.Store(&err)
r.Logger.Error("Client actions failed: ", clientActionsErrPtr)
r.Logger.Error("Client actions failed: ", err)
cancel()

// TODO: Remove or change to "always terminate when exiting early" flag
err := r.Client.TerminateWorkflow(
ctx, options.StartOptions.ID, "", "client actions failed", nil)
if err != nil {
return
if terminateErr := r.Client.TerminateWorkflow(
ctx, options.StartOptions.ID, "", "client actions failed", nil); terminateErr != nil {
r.Logger.Errorf("Failed to terminate workflow after client actions failure: %v", terminateErr)
Comment on lines -435 to +425

@THardy98 THardy98 Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why do we log here instead of return ?

@lilydoar lilydoar Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This block runs inside the go func() client-actions goroutine, so a bare return would just exit the goroutine and silently drop the terminate error — which is exactly what the old code did (if err != nil { return }). There's no error channel back to ExecuteKitchenSinkWorkflow.

The error we actually care about — the client-actions failure — does get propagated: it's stored in clientActionsErrPtr (line 418) and returned by the main path at line 435. The TerminateWorkflow call is best-effort cleanup after that failure has already happened, so its result is secondary

}
}
}()
Expand Down
10 changes: 7 additions & 3 deletions scenarios/ebb_and_flow.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,9 @@ func (e *ebbAndFlowExecutor) spawnWorkflowWithActivities(

// Start workflow.
run := e.NewRun(int(iteration))
e.RegisterDefaultSearchAttributes(ctx)
if err := e.RegisterDefaultSearchAttributes(ctx); err != nil {
return fmt.Errorf("failed to register search attributes for iteration %d: %w", iteration, err)
}
Comment on lines -284 to +286

@THardy98 THardy98 Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Likewise here, unsure if the preference would be to log or to stop the scenario with an error

options := run.DefaultStartWorkflowOptions()
options.ID = fmt.Sprintf("%s-track-%d", e.id, iteration)
options.WorkflowExecutionErrorWhenAlreadyStarted = false
Expand All @@ -303,10 +305,12 @@ func (e *ebbAndFlowExecutor) spawnWorkflowWithActivities(
// Wait for workflow completion
var result ebbandflow.WorkflowOutput
err = wf.Get(ctx, &result)
// The batch has settled either way, so drain it from the backlog the
// controller tracks; only a successful workflow counts as completed.
e.completedActivities.Add(activities)
if err != nil {
e.Logger.Errorf("ebbAndFlowTrack workflow failed for iteration %d: %v", iteration, err)
return fmt.Errorf("ebbAndFlowTrack workflow failed for iteration %d: %w", iteration, err)
}
e.completedActivities.Add(activities)
e.incrementTotalCompletedWorkflow()

@THardy98 THardy98 Jun 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is desired.

I don't have the context here (I think @dnr wrote this scenario), but it could be intended to continue through failed workflows.


return nil
Expand Down
Loading