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
3140982
Return dropped search-attribute registration error in ebb_and_flow
lilydoar Jun 25, 2026
f815015
Surface failed ebbAndFlowTrack workflows instead of swallowing them
lilydoar Jun 25, 2026
5e9e823
Fix dropped terminate error and pointer log in ExecuteKitchenSinkWork…
lilydoar Jun 25, 2026
57b2aff
Stop double-handling terminal iteration errors in generic_executor
lilydoar Jun 25, 2026
39366a3
Branch on gRPC status code, not error string, in RegisterDefaultSearc…
lilydoar Jun 25, 2026
6d9c34a
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 @@ -173,11 +173,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 @@ -404,22 +404,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 +407 to +410

@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 @@ -546,14 +534,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 -553 to +543

@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 @@ -230,7 +230,9 @@ func (e *ebbAndFlowExecutor) Run(ctx context.Context, info loadgen.ScenarioInfo)
config.MinBacklog, config.MaxBacklog, config.Period, e.Configuration.Duration)
}

e.RegisterDefaultSearchAttributes(ctx)
if err := e.RegisterDefaultSearchAttributes(ctx); err != nil {
return fmt.Errorf("failed to register search attributes: %w", err)
}

var started, completed, backlog, target int64
lastIteration := time.Now()
Expand Down Expand Up @@ -374,10 +376,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.

@lilydoar lilydoar Jul 24, 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.

It doesn't stop the scenario — returning the error feeds it into errCh, which the Run loop tolerates via MaxConsecutiveErrors (default 10): a failure increments the counter, any success resets it, and we only abort after that many consecutive failures. So we do continue through the occasional failed workflow, just not silently.

The stronger signal that failures shouldn't be swallowed is VerifyNoFailedWorkflows at the end of Run — the scenario already fails if any workflow ended up FAILED/TERMINATED, so the old log-and-continue was inconsistent with that check. It also counted failed runs as completed (incrementTotalCompletedWorkflow), which inflated TotalCompletedWorkflows (used by the == 0 guard and MinVisibilityCountEventually). Note I still drain completedActivities on failure so the backlog-oscillation math stays correct — only the completed count is gated on success now.


return nil
Expand Down
Loading