Repository navigation
fix(run): serve cached sources from the resolved document, not the unwritten output path - #2128
ThomasRooney wants to merge 2 commits into
Conversation
…written output path With --frozen-workflow-lockfile the source pipeline never writes Source.GetOutputLocation() (.speakeasy/temp/output_<hash>.yaml), yet the RunSource cache introduced in #1917 handed that path to every target after the first one that shares the source. The generator stats the missing temp file, falls through to the remote download branch of GetSchemaContents and fails with 'unsupported protocol scheme ""'. Record the document runSourceInner actually produced on the SourceResult and serve that from the cache. Only completed runs are cached: a failed source is run again by the next caller, which also restores the minimum viable spec retry (it mutates the source before re-running). The in-flight entry is removed once a run finishes for the same reason.
…nSource end to end Review follow-ups for #2128. Dropping the in-flight entry after a run opened a check-then-delete window: a caller that missed SourceResults just before the first run published its result could reach the in-flight check after the entry was deleted and register a second run of the same source. RunSource's slow path now lives in runSourceOnce, which re-checks the cache under sourceInflightMu before registering a run. runSourceInner publishes to SourceResults before RunSource removes the in-flight entry, so a missing entry means either "never ran" or "result already cached", and the re-check under the registration lock is sufficient. Lock order is sourceInflightMu -> sourceMu only; nothing takes them the other way round. Add tests that drive the real RunSource pipeline over the testdata fixtures with FrozenWorkflowLock set: - a source with an overlay shared by two targets is run once, and the second target gets an existing resolved document that differs from the never written OutputPath; - eight concurrent callers share one run; - a late caller that missed the cache before the first run completed (runSourceOnce directly) is served the completed run; this test fails on the previous commit; - a source failing mid-pipeline is re-run by the next caller and recorded once in sourceOrder; - a source failing before its pipeline starts (unknown source ref) is re-run once the workflow is fixed, as the minimum-viable-spec retry does.
|
Review follow-ups addressed in 50f7c2a: Check-then-delete race on the in-flight entry (Codex P2). Tests cover the fixed path (medium). PR description (low). Updated: the cache now hands later targets the pipeline's temp document in every mode (identical content to Verification: |
Why
Several workflows fail every
speakeasy run -t all --frozen-workflow-lockfilewithThey share one workflow shape: a source with an overlay (or several inputs / transforms, i.e. not
IsSingleInput()) that is used by more than one target, run with--frozen-workflow-lockfile. The first target generates fine; every later target fails.Root cause
internal/run/source.go:runSourceInnerskipswriteToOutputLocationwhenw.FrozenWorkflowLockis set (if !w.FrozenWorkflowLock { ... }), on purpose: frozen runs must not touch the user'soutput:file.sourceRes.OutputPathis still set toSource.GetOutputLocation(), which for a non-single-input source is.speakeasy/temp/output_<sha256(inputs)[:6]>.yaml. In a frozen run that file is never written; the real document is theoverlay_*.yaml/merge_*.yamltemp file thatrunSourceInnerreturns.RunSourcecache added in feat: add source reference resolution and merge optimizations #1917 (if c, ok := w.SourceResults[sourceID]; ok && c.OutputPath != "" { return c.OutputPath, ... }) handsOutputPathto the second and later targets sharing the source.speakeasy-core/openapi.GetSchemaContentsclassifies local vs remote withos.Stat(path) == nil; the missing temp file falls through tourl.Parse+download.DownloadFile, henceunsupported protocol scheme "".The same cache also served a "successful" result after a linting failure (
OutputPathis set before linting), which silently disabled the quickstart minimum-viable-spec retry:retryWithMinimumViableSpecmutates the source and callsRunSourceagain, but got the cached un-fixed document back.What changed
SourceResultrecords the documentrunSourceInneractually produced (documentPath, set only on success). The cache (cachedSource) serves that path instead ofOutputPath.OutputPath(the user'soutput:file or.speakeasy/temp/output_<hash>.yaml); now it returns the temp document the pipeline produced (overlay_*.yaml/merge_*.yaml/transform_*.yaml, or the input itself for a single-input source). In non-frozen runs the two files have identical content (writeToOutputLocationcopies or reformats one into the other), so the only observable difference is the path the generator is pointed at.writeToOutputLocationitself is unchanged: non-frozen runs still writeoutput:, frozen runs still do not.OutputPathis theregistry_<hash>/ download path thatNewFrozenSourcedoes not write either, so later targets sharing such a source hit the same missing-file error before this change.sourceOrderno longer records a re-run source twice.RunSource's slow path (runSourceOnce) re-checks the cache undersourceInflightMubefore registering a run. Dropping the in-flight entry after a run had opened a check-then-delete window in which a caller that missed the cache just before the first run published could register a second run of the same source.runSourceInnerpublishes toSourceResultsbefore the in-flight entry is removed, so the re-check under the registration lock closes the window; lock order issourceInflightMu -> sourceMuonly.No speakeasy-core change is needed; the
os.Statclassifier in core is correct once it is given a file that exists.Validation
go build ./... && go vet ./internal/run/... && go test -race ./internal/run/...pass. Tests ininternal/run/source_cache_test.go:TestCachedSource_*: unit tests of the cache lookup (a cached result with a missingOutputPathreturns the existing resolved document, whichopenapi.GetSchemaContentsreads locally withisRemote == false; an incomplete result is not cached).TestRunSource_*: drive the realRunSourcepipeline overtestdata/openapi.yaml+testdata/overlay.yamlwithFrozenWorkflowLockset. A source shared by two targets runs once and the second target gets an existing document that differs from the never-writtenOutputPath; eight concurrent callers share one run; a late caller that missed the cache before the first run completed is served that run (this test fails on the previous commit, where the source ran twice); a source failing mid-pipeline is re-run and recorded once insourceOrder; a source failing on an unknown source ref is re-run once the workflow is fixed, as the minimum-viable-spec retry does.End-to-end check with the CLI built from this branch against a workflow of the failing shape (one source = 1 input + 5 overlays, shared by csharp, java and typescript targets, frozen lockfile):
main): the first target generates, the second fails withfailed to get schema contents: ... unsupported protocol scheme ""on the cached.speakeasy/temp/output_<hash>.yamlpath.Running target ...for each), no schema-download error. The one remaining failure in that workflow is an unrelatedpnpm installproblem in the TypeScript target's workspace.Summary by cubic
Fixes the source cache so targets sharing a source in
--frozen-workflow-lockfileruns get the resolved document instead of the unwritten output path, which previously failed withunsupported protocol scheme ""for every target after the first.Written for commit 50f7c2a. Summary will update on new commits.