feat: emit precondition/invalid-argument ConduitError codes across pipeline, connector, orchestrator#2538
Merged
Merged
Conversation
…peline, connector, orchestrator Continues the structured-error rollout (steps 2-4 covered not-found errors in #2527-#2532) by migrating the non-not-found sentinels used to guard pipeline lifecycle and mutation preconditions: - pipeline.running / pipeline.not_running (codes.FailedPrecondition): ErrPipelineRunning / ErrPipelineNotRunning, wrapped at their clean control-plane raise sites in pkg/lifecycle/service.go (Start/Stop) and pkg/orchestrator/{pipelines,connectors,processors}.go (Update/Delete/Create guards). - pipeline.name_already_exists (codes.AlreadyExists) / pipeline.name_missing (codes.InvalidArgument): pkg/pipeline/service.go Update and validatePipeline (the latter joined via cerrors.Join, same pattern as pkg/provisioning/config/validate.go). - connector.running (codes.FailedPrecondition): the single choke point, Instance.Connector in pkg/connector/instance.go. - connector.invalid_type (codes.InvalidArgument): the user-facing check in connector.Service.Create. - orchestrator.invalid_processor_parent_type (codes.InvalidArgument), orchestrator.pipeline_has_{processors,connectors}_attached, orchestrator.connector_has_processors_attached, orchestrator.immutable_provisioned_by_config (all codes.FailedPrecondition): new pkg/orchestrator/codes.go, wrapped at every raise site in the orchestrator package. Every sentinel stays in the error chain via conduiterr.Wrap (invariant comment at each site); existing errors.Is checks are preserved, though tests that previously asserted strict `is.Equal(err, sentinel)` identity had to be updated to errors.Is + a ConduitError code/suggestion check, since wrapping necessarily changes the concrete error value (documented in the PR). Deferred (reported, not migrated, to keep blast radius tight): the lifecycle-poc (PipelineArchV2 preview) mirror of ErrPipelineRunning/ ErrPipelineNotRunning; the internal runPipeline tomb-alive defensive check; and three defensive/unreachable ErrInvalidConnectorType switch defaults (SetState, store.decode, Instance.Connector) guarding already-validated data. Design: docs/design-documents/20260705-conduit-error-and-structured-output.md Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tapg9dLWXKMZJoL65R24vd
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Continues the structured-error rollout (item 2). Steps 1-4 (#2524, #2527, #2528, #2530,
#2532) wired the
ConduitErrorfoundation into the API boundary and migrated everynot-found sentinel. This PR migrates the non-not-found sentinels that guard pipeline
lifecycle and mutation preconditions, following the exact
conduiterr.Wrappattern fromthose PRs.
Purely additive:
pkg/http/api/status/status.go'sconduitErrorStatusalready turns any*ConduitErrorinto the right gRPC status +ErrorInfodetail; the sentinel-basedcodeFromError/PipelineError/ConnectorErrorswitches are untouched and remain thefallback for anything not yet migrated. Every sentinel stays in the error chain via
conduiterr.Wrap, soerrors.Is(err, ErrX)checks throughout the codebase areunaffected — verified below.
Codes added
pipeline.runningErrPipelineRunningCodePipelineRunning)pipeline.not_runningErrPipelineNotRunningCodePipelineNotRunning)pipeline.name_already_existsErrNameAlreadyExistsCodePipelineNameAlreadyExists)pipeline.name_missingErrNameMissingCodePipelineNameMissing)connector.runningErrConnectorRunningCodeConnectorRunning)connector.invalid_typeErrInvalidConnectorTypeCodeConnectorInvalidType)orchestrator.invalid_processor_parent_typeErrInvalidProcessorParentTypecodes.go)orchestrator.pipeline_has_processors_attachedErrPipelineHasProcessorsAttachedorchestrator.pipeline_has_connectors_attachedErrPipelineHasConnectorsAttachedorchestrator.connector_has_processors_attachedErrConnectorHasProcessorsAttachedorchestrator.immutable_provisioned_by_configErrImmutableProvisionedByConfigPer-sentinel raise sites: migrated vs deferred
ErrPipelineRunning/ErrPipelineNotRunning— sentinel is defined inpkg/pipelinebut raised in
pkg/lifecycleandpkg/orchestrator. Migrated the clean control-planesites:
lifecycle.Service.Start/Stop(the authoritative check) and everyorchestrator.{Pipeline,Connector,Processor}OrchestratorCRUD guard (9 sites forErrPipelineRunning, 2 forErrPipelineNotRunning).Deferred (reported, not touched): the
lifecycle-poc(PipelineArchV2previewengine) mirror of both Start/Stop checks, and the internal
lifecycle.Service.runPipelinetomb-alive defensive check (redundant withStart'scheck, several layers below the API boundary). Both remain bare sentinels — no
behavior change,
errors.Isstill holds.ErrNameAlreadyExists/ErrNameMissing(pipeline package only —connector.ErrNameMissingis a distinct sentinel, out of scope per the task) —migrated both raise sites:
Service.Update(direct return) andvalidatePipeline(joined via
cerrors.Join, same pattern already proven inpkg/provisioning/config/validate.go'sfieldErrorhelper).ErrConnectorRunning— single choke point,connector.Instance.Connector.Migrated.
ErrInvalidConnectorType— 4 raise sites total. Migrated the one user-facing site,connector.Service.Create(validates a caller-supplied type). Deferred: the threedefault:branches inService.SetState,Store.decode, andInstance.Connector— all guard against aTypevalue already validated atCreatetime (storage-layer/defensive, effectively unreachable in practice); left as bare
sentinels.
ErrInvalidProcessorParentType,ErrPipelineHasProcessorsAttached,ErrPipelineHasConnectorsAttached,ErrConnectorHasProcessorsAttached,ErrImmutableProvisionedByConfig— all defined and raised entirely withinpkg/orchestrator(3, 1, 1, 1, and 9 sites respectively). Migrated every site; addedsmall unexported helpers (
pipelineRunningErr,immutableProvisionedByConfigErr,invalidProcessorParentTypeErrin the newpkg/orchestrator/codes.go) to avoidrepeating the wrap+suggestion boilerplate across the 9
ErrImmutableProvisionedByConfigsites.
Adversarial self-review
conduiterr.Wrapcall:Wrap'smsgreplaces (does notconcatenate with) the cause's
Error()text, so for sites that previously didcerrors.Errorf("<prefix>: %w", sentinel)I reconstructed the equivalent text viafmt.Sprintf/string-concat withsentinel.Error()so the human-readable message isunchanged.
pkg/pipeline/service.go'svalidatePipelinejoins multiple wrapped*ConduitErrors viacerrors.Join;conduiterr.Get(viaerrors.As) still finds thefirst one, matching the existing
pkg/provisioning/config/validate.goprecedent (Go1.20+
Unwrap() []errorsupport).is.Equal(err, sentinel),stronger than
errors.Is) on sentinels that used to be returned bare:TestPipelineOrchestrator_{Update,Delete,UpdateDLQ}_PipelineRunning,TestPipelineOrchestrator_Delete_PipelineHas{Processors,Connectors}Attached,TestConnectorOrchestrator_{Delete,Update}_PipelineRunning,TestProcessorOrchestrator_{CreateOnPipeline,UpdateOnPipeline,DeleteOnPipeline}_PipelineRunning.Wrapping necessarily breaks
is.Equalidentity (the concrete error value changes fromthe sentinel to a
*ConduitError); updated these toerrors.Is+ a newconduiterr.Getcheck for the code and suggestion — this is not a weakenedassertion, it verifies the same fact (this exact sentinel is in the chain) the way
every other migrated site's test does.
TestInstance_Connector_AlreadyRunning(connector),TestServiceLifecycle_Start_AlreadyRunning/
TestServiceLifecycle_Stop_NotRunning(lifecycle).pkg/http/api/status/status.go— its sentinel-based fallback switchesare intentionally left alone per the established pattern (they still catch the
deferred/bare-sentinel sites above).
Test plan
go build ./...go test ./pkg/pipeline/... ./pkg/connector/... ./pkg/orchestrator/... ./pkg/http/api/... ./pkg/provisioning/... -count=1— all greengo test -raceon the same packages plus./pkg/lifecycle/...— all green, no data racesgolangci-lint run ./pkg/pipeline/... ./pkg/connector/... ./pkg/orchestrator/...— 0 issuesGOOS=linux GOARCH=386 go build ./...— OKerrors.Is(err, ErrX)check across the repo still holds for allmigrated and deferred sentinels (grepped for
Is(usages before and after)Risk tier: Tier 2 (API/orchestrator error surface, not a data-path/serialization
change — no wire format, no protocol, no state layer touched). One reviewer approval
required per
CLAUDE.md; not merging this myself per the task instructions.🤖 Generated with Claude Code
https://claude.ai/code/session_01Tapg9dLWXKMZJoL65R24vd