feat(cli): add conduit doctor preflight diagnostics#2576
Merged
Conversation
Implements the design doc and ADR: - docs/design-documents/20260707-cli-doctor.md - docs/architecture-decision-records/20260707-check-engine-shared-by-doctor-and-scaffold.md (brought over from docs/v0.17-first-wave-design, not yet on main, since doctor's implementation needs them in-tree; docs/cli-output-conventions.md is included too as doctor's required output contract). Adds `cmd/conduit/root/doctor/doctorcheck` (pure, no cobra/ecdysis import — proven by an engine-only test) supplying doctor's check set on top of the already-merged pkg/conduit/check engine: config.resolve, config.validate (reuses Config.Validate()), store.reachable (reuses the new conduit.OpenStore, extracted verbatim from NewRuntime), network.grpc/http (check.AddrBindable), plugins.connectors_dir/processors_dir, plugins.builtin, engine.reachable (api.Client), and plugins.standalone_compat behind --deep. `cmd/conduit/root/doctor` is the thin cobra wrapper (DoctorCommand + a hidden `__plugin-spec-check` worker command) and human renderer, wired into root.go. Review must-fixes applied: - Exit code: doctor reduces N check results to one process exit code via the shared check.Report.ExitCode() (max bucket), not reimplemented. Getting that reduction to actually reach the process exit code (despite ExecuteWithResult correctly returning a nil error for a domain finding, per the CLI output conventions) needed a small, generally-useful addition to cecdysis: CommandWithResultExitCode, plus cli.go reading it off the leaf *cobra.Command via ExecuteC(). This is additive and backward compatible — existing CommandWithResult commands are unaffected. - --deep's plugins.standalone_compat isolates each plugin dispense+specify call in a subprocess (re-execs the running conduit binary as the hidden `__plugin-spec-check` command) with its own timeout, since go-plugin's background goroutines can panic in the calling process in a way recover() cannot catch. An abnormal child exit (nonzero, killed, crashed) is treated as Fail. - engine.reachable is documented as binary pass/fail — CheckHealth cannot distinguish "no server" from "unhealthy". - Flags: --json, --check <name> (repeatable; unknown name or a --deep-gated name without --deep is a HARD failure, exit 2), --deep, --require-server, --quiet, --no-color. - store.reachable is side-effect-free: it records whether the configured badger/sqlite path existed before opening the store and removes only what it itself created, never touching pre-existing state. Adversarial self-review findings (see PR description for full notes): config.resolve cannot observe "unparseable config file" (ecdysis's own config parsing fails before doctor's checks ever run) — documented as a known limitation, not silently claimed as covered. configResolveCheck distinguishes a missing file from an unreadable one rather than collapsing both into the same message. Roadmap: Phase 1, CLI-as-product execution plan §3. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tapg9dLWXKMZJoL65R24vd
MD049 (emphasis style: underscore, not asterisk), MD040 (fenced code blocks need a language), and MD032 (lists need surrounding blank lines) — caught by CI's markdownlint-cli2 check on the docs brought in from docs/v0.17-first-wave-design. Verified against the exact pinned CI version (markdownlint-cli2 0.18.1 / markdownlint 0.38.0): 0 errors across all docs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Tapg9dLWXKMZJoL65R24vd
# Conflicts: # docs/design-documents/20260707-cli-doctor.md # docs/design-documents/20260707-cli-output-conventions.md
This was referenced Jul 8, 2026
devarismeroxa
added a commit
that referenced
this pull request
Jul 8, 2026
… fix CI flake (#2578) Fixes the CI flake noted on #2534 that failed the `test` job on PRs #2576 and #2577 (TestServiceLifecycle_PipelineError, TestServiceLifecycle_PipelineStop). The immediate trigger was a raw time.Sleep(100ms) used to let 10 records "finish flowing" before stopping the pipeline in TestServiceLifecycle_PipelineStop and its sibling TestServiceLifecycle_PipelineSuccess. Under CI load 100ms isn't always enough, so Stop/StopAll can race the data path, under-deliver records, and fail the source/destination mocks' record-count assertions in t.Cleanup. Both are replaced with waitForRecordsAcked, which polls the source connector's exported, lock-guarded State field (set by Source.Ack) until every record has actually been acked, instead of guessing a duration. Digging into *why* the flake also showed a bare status mismatch ("SystemStopped != Running") turned up four distinct, real concurrency bugs in pkg/lifecycle-poc, found and fixed via `-race -shuffle=on` repetition: 1. service.go WaitPipeline: a lookup-after-delete race could return a false nil instead of the pipeline's terminal error/status if the cleanup goroutine removed the pipeline from runningPipelines between the lookup and return. This is the identical bug already diagnosed and fixed in the sibling pkg/lifecycle package (see docs/design-documents/20260706-forceful-stop-test-determinism.md); ported the same terminalErrors-map fix here. 2. service.go runPipeline: the worker goroutine's tomb.Kill(err) for a fatal error only happened in tomb.v2's own post-return bookkeeping, which raced the cleanup goroutine waking from workersWg.Wait() and reading rp.t.Err() — losing the race made a fatal stop get reported as a clean graceful stop. Fixed by killing the tomb synchronously before returning. 3. service.go runPipeline (the actual root cause of the CI-observed "SystemStopped != Running"): the function's own initial UpdateStatus(StatusRunning) call and the cleanup goroutine's terminal UpdateStatus call are not safe to run concurrently on the same *pipeline.Instance (SetStatus is lock-guarded, but the Error field write and the store's JSON-encode of the whole struct for persistence are not). With mocks providing zero I/O latency, the cleanup goroutine could call its own UpdateStatus while the initial one was still mid-flight and win the race, clobbering a correct terminal status back to "running". Confirmed by a genuine `-race` data race report. Fixed by gating the cleanup goroutine's UpdateStatus on a channel closed only after the initial one fully completes (kept the two tomb.Go registrations adjacent to avoid a related tomb.Go-after-death panic that a naive reordering introduced). 4. funnel/worker.go Worker.Stop: tore down the source (nils the plugin) before setting w.stop, so a concurrent Read() racing the stop could observe plugin.ErrPluginNotRunning before doTask's graceful-stop check (w.stop.Load()) saw stop=true, misreporting a graceful stop as a real failure. Fixed by setting w.stop before tearing down. Also added the missing `defer persister.Wait()` to PipelineError/PipelineStop (PipelineSuccess already had it) — without it the persister's background flush goroutine can log via log.Test(t) after the test function returns, which panics and can corrupt whatever test happens to be running concurrently in the same process. Verification (pkg/lifecycle-poc/): - go test -run 'TestServiceLifecycle' -race -count=500: 0 failures - yes >/dev/null (x3) & go test -run 'TestServiceLifecycle' -count=300: 0 failures - go test ./... -race -shuffle=on -count=50: 0 failures - go build ./...: clean - golangci-lint run ./pkg/lifecycle-poc/...: 0 issues Self-review: re-read the diff for error paths, concurrency, and resource cleanup. Confirmed close(startupDone) is unconditional (both the success and UpdateStatus-error paths reach it), so the cleanup goroutine can never hang. Confirmed both tomb.Go registrations stay adjacent so tomb.alive can't reach 0 before the cleanup goroutine exists. Not fixed here (flagged for a follow-up, out of scope for a test-flake PR): pkg/lifecycle-poc's force-stop path kills the tomb with a bare pipeline.ErrForceStop instead of cerrors.FatalError(...) like pkg/lifecycle does, and the "recovery not implemented" branch in the cleanup switch is a complete no-op that leaves a dead pipeline's status stuck at "running" forever instead of reporting Degraded — both are real gaps but change lifecycle semantics beyond what a flaky-test fix should carry. Roadmap: Phase 0 stabilization — CI reliability for the arch-v2 (funnel) pipeline lifecycle package ahead of it replacing pkg/lifecycle. Claude-Session: https://claude.ai/code/session_01Tapg9dLWXKMZJoL65R24vd Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
Implements
conduit doctorperdocs/design-documents/20260707-cli-doctor.mdand the ADRdocs/architecture-decision-records/20260707-check-engine-shared-by-doctor-and-scaffold.md. Both docs (plus the shareddocs/design-documents/20260707-cli-output-conventions.mddoctor must follow) are brought into this PR fromdocs/v0.17-first-wave-design, which isn't merged tomainyet — doctor's implementation needs them in-tree to be reviewable against something.doctoris an offline, non-destructive preflight answering "wouldconduit runsucceed here?" — config resolution/validation, database reachability, API address availability, plugin directories, the built-in plugin registry, and (optionally) whether a running engine is reachable. It does not boot a Runtime.What's here
pkg/conduit/runtime.go: extractedOpenStore(cfg, logger)verbatim fromNewRuntime's DB-open switch (behavior-preserving;NewRuntimenow just calls it). This is the one edit to existing runtime code, kept surgical per the task note about a parallel PR touching nearby files.cmd/conduit/root/doctor/doctorcheck(new, pure — no cobra/ecdysis import, proven by an engine-only test that callsDefaultChecks+check.Rundirectly): the check set —config.resolve,config.validate(reusesConfig.Validate()),store.reachable(reusesconduit.OpenStore),network.grpc/network.http(reusescheck.AddrBindable),plugins.connectors_dir/plugins.processors_dir,plugins.builtin,engine.reachable(reusesapi.Client), andplugins.standalone_compatbehind--deep.cmd/conduit/root/doctor(new):DoctorCommand(the cobra wrapper + human renderer) and a hidden__plugin-spec-checkworker command--deepre-execs itself as (see isolation note below). Registered inroot.go.cmd/conduit/cecdysis: addedCommandWithResultExitCode, a small additive interface +cli.gowiring (ExecuteCinstead ofExecute, reading a cobraAnnotationsvalue back off the leaf command). See "Structural gap found" below for why this was necessary.README.md: new "Preflight checks" section, plus a note in "Exit codes" about multi-result aggregation.Review must-fixes (from the design doc's own review) applied
pkg/conduit/check.Report.ExitCode()(already-merged, already-tested max-bucket logic) — not reimplemented.--deep'splugins.standalone_compatisolates each plugin's dispense+specify call in a subprocess (re-execs the runningconduitbinary as__plugin-spec-check <path>) with a 15s per-plugin timeout, becausehashicorp/go-pluginspawns background goroutines in the calling process that arecover()cannot catch across a goroutine boundary. Any abnormal child exit (nonzero, timeout-killed, crashed) is treated asFailfor that plugin — verified manually against both a plugin binary that exits 1 and a garbage (non-executable-format) binary;doctoritself never crashes.engine.reachableis documented, not glossed over, as binary pass/fail —CheckHealthcan't distinguish "no server" from "unhealthy."--json,--check <name>(repeatable; an unknown name, or a--deep-gated name passed without--deep, is a HARD failure listing valid names, exit 2),--deep,--require-server,-q/--quiet,--no-color.store.reachablehas no side effects: it records whether the configured badger/sqlite path existed before opening the store, and removes only what it itself created afterward — pre-existing state (a real priorconduit run) is never touched. Covered by both adoctorcheck-level test and a CLI-level end-to-end test.A structural gap I found and fixed (not anticipated by the design doc)
The already-merged
cecdysis.CommandWithResultDecoratorhas exactly two paths:ExecuteWithResultreturns an error (a HARD failure — sets the JSON envelope'serrorfield,RunEreturns that error) or it returnsnil(renders normally,RunEreturnsnil). But per the CLI output conventions, a domain finding (Outcome.OK == false, e.g. doctor's checks failing) must return a nil error fromExecuteWithResult— the envelope'serrormust staynull. With the decorator as merged, that nil-error path meantcmd.Execute()always returnednil, socli.Run'sos.Exit(exitcode.ExitCode(err))was always 0, regardless of how many checks failed. There was no way to satisfy "exit 2/3 on failing checks" and "envelope error stays null on a domain finding" simultaneously with the merged decorator.Fix: added
cecdysis.CommandWithResultExitCode(a small, optional, additive interface:ExitCode(Outcome) int). When implemented andExecuteWithResultreturnsnil, the decorator records the exit code as acobra.Command.Annotationsentry on the leaf command (there's no other channel available post-hoc without changingRunE's return value, which would defeat the "envelope error stays null" requirement).cmd/conduit/cli.gonow callscmd.ExecuteC()(returns the leaf command) instead ofcmd.Execute()and reads the annotation back if the top-level error was nil. This is additive and backward compatible — no existingCommandWithResultcommand implements the new interface, so nothing else changes behavior. Covered by new tests incecdysis/exitcode_test.goand end-to-end indoctor_test.go.This will matter again for the
pipelines validatedesign doc, which has the identical "multi-result command, exit code must reflect findings, envelope error must stay null" shape.Adversarial self-review
config.resolvecannot observe "unparseable config file." By the time doctor's checks run,ecdysis's own config-parsing (ParseConfig, called fromCommandWithConfig'sPreRunE) has already resolvedc.Cfg— a malformed YAML file fails there, as a HARD failure, beforeExecuteWithResult(and therefore any check) ever runs. Documented onconfigResolveCheck's type doc rather than silently claimed as covered; the design doc's three-way "found+parsed / defaults-used / unparseable" split degrades to a two-way "found / not found" split for this reason.configResolveCheck's stat-error branch originally collapsed "genuinely missing" and "some other stat error" (permission denied, etc.) into the same "no config file" message — fixed during self-review to distinguish them (os.IsNotExistvs. a generic fallback), so a permissions problem doesn't get misreported as "file doesn't exist."log.level→ fail/exit 2, missing plugin dirs → warn/exit 0, all-good → exit 0/ok:true,--jsonenvelope shape,--checkfiltering (including the standalone_compat-without---deepguard),--deepagainst both a broken and a garbage plugin binary (no crash), and noconduit.db/badger directory left behind after any of the above runs.plugins.standalone_compatis connector-only. Standalone processors are WASM (wazero, in-process), notgo-pluginsubprocesses — a different risk profile than the one the review flagged (background-goroutine panics in the calling process). Scoped--deepto connectors only rather than inflating this PR into wazero-compile-validation territory; documented as a deliberate scope decision, not an oversight.llms.txtorconduit.iodocs-source update: neither exists in this repo (llms.txtisn't present anywhere; theconduit.iodocs site is a separate repo not reachable from here). Added a "Preflight checks" section toREADME.mdinstead, and left a pointer to the design doc.Test plan
go build ./...— clean.go test ./cmd/conduit/... ./pkg/conduit/... -race -count=1— all green, including:cmd/conduit/root/doctor/doctorcheck— the pure, engine-only suite (bad badger path → fail/exit 3, port-in-use → fail/exit 3, missing plugin dir → warn/exit 0, invalid config field → fail/exit 2, all-good → exit 0, no DB dir left behind — both the "fresh path" and "pre-existing path is never deleted" cases, in-memory DB warns, API-disabled warns,--require-serverfails, a panickingcheck.Checkmixed into doctor's real check set is isolated asFailwithout crashing the test binary,CheckNames()covers every nameDefaultCheckscan produce).cmd/conduit/root/doctor— the cobra-level integration suite (same scenarios driven through the realExecuteC()path, provingCommandWithResultExitCodeend-to-end: JSON envelopeerror:null+ exit 2 on a domain finding, unknown--checkname as a HARD failure,--deep-gated name without--deep,--checkfiltering,--quietsuppressing pass but not warn lines, no DB dir left behind at the full-CLI layer).cmd/conduit/cecdysis— new tests forCommandWithResultExitCode/ResultExitCode(annotation set on a domain failure, not set on success, not set for a zero code, nil/unset/corrupted-annotation handling).golangci-lint run ./cmd/conduit/... ./pkg/conduit/...— 0 issues.🤖 Generated with Claude Code
https://claude.ai/code/session_01Tapg9dLWXKMZJoL65R24vd