Error-handling cleanup: surface swallowed failures, branch on values not strings#411
Error-handling cleanup: surface swallowed failures, branch on values not strings#411lilydoar wants to merge 6 commits into
Conversation
spawnWorkflowWithActivities ignored the error from RegisterDefaultSearchAttributes, so a registration failure was silently lost and surfaced later as an obscure workflow-start failure.
spawnWorkflowWithActivities logged wf.Get failures and returned nil, so they never reached the MaxConsecutiveErrors guard and a failed workflow was still counted as completed (inflating TotalCompletedWorkflows, which post-scenario verification relies on). Return the error and gate the completion count on success; the backlog still drains either way so the controller's behavior is unchanged.
…flow The client-actions goroutine logged the atomic.Pointer rather than the error value, and silently dropped the result of the cleanup TerminateWorkflow call. Log the actual error, and log the terminate failure (the goroutine has no return path for it).
The per-iteration goroutine logged the terminal failure and also sent it on doneCh, where Run returns it wrapped for its caller to report. Drop the goroutine log for the terminal branch (propagate only); keep the retry-branch log, whose transient error is superseded and never returned.
…hAttributes
Replace strings.Contains(err.Error(), ...) with status.Code(err) ==
codes.AlreadyExists, matching ensureNexusEndpoint in the same file. The
second match string was a typo ("unavailble") that never matched, so no
working behavior is lost.
Preserve the error chain at this library boundary with %w, matching the sibling wraps in the same function (lines 93, 118).
| // 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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 fmt.Errorf("ebbAndFlowTrack workflow failed for iteration %d: %w", iteration, err) | ||
| } | ||
| e.completedActivities.Add(activities) | ||
| e.incrementTotalCompletedWorkflow() |
There was a problem hiding this comment.
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.
| e.RegisterDefaultSearchAttributes(ctx) | ||
| if err := e.RegisterDefaultSearchAttributes(ctx); err != nil { | ||
| return fmt.Errorf("failed to register search attributes for iteration %d: %w", iteration, err) | ||
| } |
There was a problem hiding this comment.
Likewise here, unsure if the preference would be to log or to stop the scenario with an error
| 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) |
There was a problem hiding this comment.
why do we log here instead of return ?
There was a problem hiding this comment.
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
2 participants
What
A focused error-handling cleanup across the omes Go code, prompted by an audit against our Go error-handling conventions (don't drop errors, log XOR return, branch on values not strings, propagate at boundaries). Each fix is its own commit. Scope was limited to High and Medium severity findings — bugs and correctness issues — not the broad stylistic layer (noise-prefix messages, capitalized error strings).
Changes (one commit each)
1. Return dropped search-attribute registration error (
scenarios/ebb_and_flow.go)spawnWorkflowWithActivitiescalledRegisterDefaultSearchAttributes(ctx)and ignored itserror. A registration failure was lost and only surfaced later as an obscure workflow-start error. The sibling caller ingeneric_executor.goalready checks this return value.2. Surface failed
ebbAndFlowTrackworkflows instead of swallowing them (scenarios/ebb_and_flow.go)wf.Getfailures were logged and thenreturn nil, so they never reached theMaxConsecutiveErrorsguard, and a failed workflow was still counted as completed — inflatingTotalCompletedWorkflows, which post-scenario verification relies on. Now the error is returned and the completion count is gated on success. The backlog still drains on either outcome, so the rate controller's behavior is unchanged.3. Fix dropped terminate error and pointer log (
loadgen/scenario.go)In
ExecuteKitchenSinkWorkflow's client-actions goroutine, the failure log printed theatomic.Pointer[error]value (an address) rather than the error, and the result of the cleanupTerminateWorkflowcall was silently discarded. Now it logs the actual error, and logs the terminate failure (the goroutine has no return path to propagate it).4. Stop double-handling terminal iteration errors (
loadgen/generic_executor.go)The per-iteration goroutine logged a terminal failure and sent it on
doneCh, whereRunreturns it wrapped for its caller to report — the same error handled twice. The terminal branch now propagates only. The retry-branch log is kept on purpose: that error is superseded by the retry and never returned, so the log is its only record.5. Branch on gRPC status code, not error string (
loadgen/scenario.go)RegisterDefaultSearchAttributesdecided whether to ignore an "already exists" failure viastrings.Contains(err.Error(), ...). Replaced withstatus.Code(err) == codes.AlreadyExists, matchingensureNexusEndpointin the same file. The second match string was a typo ("unavailble") that could never match the real message, so no working behavior is lost.6. Wrap cert-load error with
%w(clioptions/client.go)loadTLSConfigused%sto format the cert-load error, severing the unwrap chain at this library-ish boundary. Switched to%w, matching the two sibling wraps in the same function.Testing
go buildclean across all first-party packages.go test ./clioptions/ ./scenarios/— pass.go test ./loadgen/worker-free unit tests (cover thegeneric_executorchange) — pass.SDK=go go test ./loadgen/ -run TestKitchenSink— pass (ok … 25.5s); exercises theExecuteKitchenSinkWorkflowandRegisterDefaultSearchAttributespaths against a real devserver.Note: a no-
SDKgo test ./loadgen/fails locally becauseTestKitchenSinkruns the full SDK matrix (Go/Java/Python/Ruby/TS/.NET) and only the Go worker toolchain is built here. That's environmental and pre-existing — CI setsSDKper the test's own skip logic.