fix(embed): lazy DB open + skip file provisioning on empty PipelinesDir (B1 DX hardening)#2676
Conversation
…ir (B1 DX hardening) A DX audit of the merged libconduit B1 API (#2667) found two frozen-Options- contract bugs worth a fast-follow while adoption is still zero: 1. Leaving Options.PipelinesDir empty fell through to pkg/conduit's CLI- oriented cwd/pipelines default. Since that directory usually doesn't exist for a pure-embed caller, provisioning then failed as a swallowed ERROR log while Run still returned a healthy Handle — contradicting New's doc promise that every failure is returned as an error. Fixed by adding pkgconduit.Config.Pipelines.Disabled (no CLI flag, zero value false, so `conduit run` is unaffected) and having the embed layer's New set it whenever PipelinesDir is empty, short-circuiting file provisioning entirely instead of scanning cwd. A genuine provisioning failure with a *configured* PipelinesDir now surfaces as a returned error from Run via a new, purely-additive Runtime.ProvisioningErr field the CLI never reads. 2. New eagerly opened the database, so a constructed-but-never-Run Engine leaked a Badger file lock / Postgres pool, and Close was mandatory even without Run. Fixed by deferring pkgconduit.NewRuntime construction to the first Run or Import call (ensureRuntime); New now only validates Options. Close is a safe no-op if nothing was opened. TestNew_StoreAlreadyOpen_ ReturnsCodedError is updated: the contention now has to be forced via Run, since New no longer opens anything. A runtime.SetFinalizer safety net logs a leak warning (through the injected logger, else slog.Default()) if an Engine that did open resources is GC'd without Close. Updates doc.go and example_test.go for the lazy-open timing change, and adds regression tests for both fixes (empty-PipelinesDir never scans cwd, a configured-but-malformed PipelinesDir surfaces as an error, and the metrics/ store-collision tests moved from New to Run since that's where opening now happens). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015GQFzakPShAYj8CcwajYDD
Review found: ensureRuntime never checked the closed flag, so calling Run or Import on an Engine after Close (with no prior Run/Import) silently opened a fresh runtime and database the caller believed was already released. ensureRuntime now checks closed first and returns the existing conduiterr.CodeInvalidArgument (same code used for double Run / nil-Handle Stop) instead of minting a new one. Also documents, as known v1 behavior (no code change), that only the first caller's ctx bounds the lazy DB dial: a concurrent Import with a short-lived ctx can cause a concurrent Run's dial to observe that cancellation. Regression test: New -> Close -> Run/Import returns the coded error and opens no database (verified via a second Engine at the same BadgerDB path opening without contention). Confirmed to fail without the fix and pass with it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015GQFzakPShAYj8CcwajYDD
30a3dca to
16c6896
Compare
|
Applied the review-requested fix, rebased onto current `main` (no conflicts — disjoint from #2680/#2674). 1. Use-after-close guard in 2. Regression test: 3. Known-behavior doc (no code change): Gates run locally: Not merging — Tier-1-adjacent (data-path lifecycle), needs human sign-off per CLAUDE.md. |
Summary
DX audit fast-follow on the merged libconduit B1 embed API (#2667). Adoption is zero, so now is the right time to fix two frozen-
Options-contract issues rather than carry them forward.Fix 1 — silent provisioning failure on empty
Options.PipelinesDir. LeavingPipelinesDirempty fell through topkg/conduit's CLI-orientedcwd/pipelinesdefault. That directory usually doesn't exist for a pure-embed caller, so provisioning failed as a swallowed ERROR log whileRunstill returned a healthyHandle— contradictingNew's doc promise that every failure is returned as an error.pkgconduit.Config.Pipelines.Disabled(nolong/mapstructuretag — not a CLI flag, zero valuefalse,conduit runnever sets it). The embed layer'sNewsets it wheneverOptions.PipelinesDir == "", which short-circuitsinitServices's call toProvisionService.Initentirely instead of falling back to a scan.pkgconduit.Runtime.ProvisioningErr— purely additive, set regardless ofExitOnDegraded, never read by the CLI.Engine.Runreads it right afterReadyfires; if set, it cancels + drains (Invariant 7) and returns the error instead of aHandle. A genuine failure with a configuredPipelinesDir(missing dir, malformed YAML) now surfaces as a returned error, matchingconduit run's own log-and-continue default being intentionally NOT reused on the embed path.Disabledis alwaysfalseon that path, andProvisioningErris never consulted there.Fix 2 — eager DB open / mandatory Close footgun.
Newopened the database eagerly, so a constructed-but-never-RunEngineleaked a Badger file lock or Postgres pool, andClosewas mandatory even withoutRun.Newnow only validatesOptions(including callingcfg.Validate()directly, so a bad config still fails fast). Constructingpkg/conduit'sRuntime— which is where the DB actually opens — is deferred toensureRuntime, called lazily on the firstRunorImport(also wired intoStartPipeline/StopPipelineso a runtime-never-built Engine can't nil-pointer-panic on those either).Closereads whether anything was actually opened and is a safe no-op otherwise; when something was opened it still delegates toRuntime.CloseDB(feat(embed): libconduit B1 Go embedding API (New/Engine/Handle/Import) #2667's idempotent double-release path, unchanged).runtime.SetFinalizersafety net onEngine: if an Engine that DID open resources is GC'd withoutClose, it logs a leak warning through the injected logger (elseslog.Default()). Verified working end-to-end with a standalone smoke test (construct → Run → Stop → drop the reference → GC → warning observed in the log). Close clears the finalizer.TestNew_StoreAlreadyOpen_ReturnsCodedErroris preserved but the contention now has to be forced viaRun(both engines), sinceNewalone opens nothing — same for the metrics-collision and metrics-cross-talk tests, which also moved their trigger fromNewtoRun.Store-already-open timing change
Documented explicitly in
Engine's "Lifecycle contract" doc and in the updated test: the codedCodeUnavailablecollision now surfaces "when the DB actually opens (on Run/Import)," not atNew.Failure-mode analysis (Tier-1-adjacent: frozen
Optionssemantics + DB lifecycle + provisioning behavior)Neweagerly failing on a badPipelinesDir/DB config would now see that surface atRun/Importinstead — mitigated by keepingcfg.Validate()synchronous inNew(a nonexistent customPipelinesDirstill fails fast atNew, proven byTestNew_NonexistentCustomPipelinesDir_FailsFast). A caller relying on metrics being registered immediately afterNew(beforeRun) would see a behavior change — there is no such use case in the codebase (adoption is zero) and it's the described intent of this fix.Example_helloPipeline) and its CI wall-clock budget test are the closest thing to a canary and both still pass.ensureRuntime/Closeshare onesync.Mutex(notsync.Once+atomics) specifically so aCloseracing the very firstRun/Importblocks until that open attempt resolves, rather than observing "nothing opened yet" and returning early while an open completes moments later with nothing left to release it — documented inClose's own doc as the one remaining caller-orchestrated race (callingCloseconcurrently with the firstRun/Import, as opposed to a later one, which just reuses the cached result).Close.Adversarial self-review
Config.Pipelines.Disabledhas no CLI flag tag and defaultsfalse, soconduit runis provably unaffected (initServices' new branch is unreachable from the CLI).Runtime.ProvisioningErrdoesn't change what's logged or whenconduit runitself fails — it's an added field next to unchanged control flow, not a rewrite of it.opened/closedmutex-guarded reads inCloseand the finalizer to confirm no data race (ran the full suite under-race).TestRun_BadPipelinesDir_ReturnsErrorassumed a nonexistentPipelinesDirwould fail atRun— it actually fails atNew(cfg.Validate()'s existing non-default-path stat check), which is a better outcome than the bug report implied for that specific sub-case. Split it into two tests:TestNew_NonexistentCustomPipelinesDir_FailsFast(New-time) andTestRun_MalformedPipelinesDir_ReturnsError(an existing directory with unparseable YAML — the case that genuinely can only be discovered at Run/Init time).StartPipeline/StopPipelinenow also callensureRuntime— not strictly requested, but necessary to avoid a new nil-pointer-panic footgun these lazy semantics would otherwise introduce for a caller that invokes them before anyRun/Import.B2 conflict — sequencing note for DeVaris
This will conflict with the in-flight B2 builder PR #2673 (
feat/embed-b2-pipeline-builder, currently open) onconduit.go— confirmed viagit diff main origin/feat/embed-b2-pipeline-builder -- conduit.go, which touches the same file (addsImportPipelinenearImport). Recommend merging whichever lands first, then rebasing the other before merge — I have not attempted to resolve that conflict here.Test plan
make generate && git diff --exit-code— clean, no driftgolangci-lint run ./... --new-from-rev=851c406— 0 issuesgo build ./...go test -race ./ ./pkg/conduit/... ./pkg/provisioning/...— all green, including feat(embed): libconduit B1 Go embedding API (New/Engine/Handle/Import) #2667's Close tests (updated for lazy semantics) and new regression tests for both fixesDO NOT MERGE — flagging for DeVaris awareness per frozen-contract-semantic changes and the B2 sequencing conflict above.
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_015GQFzakPShAYj8CcwajYDD