From 962fb751a9657fd2d645c5cf3986888641a5a60a Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 9 Aug 2026 03:50:07 +0300 Subject: [PATCH 1/2] docs(engine): state and pin the concurrent-use guarantee --- compilers/compilers.go | 10 +++ engine/concurrency_test.go | 141 +++++++++++++++++++++++++++++++++++++ engine/engine.go | 13 ++++ pass/validate.go | 4 ++ 4 files changed, 168 insertions(+) create mode 100644 engine/concurrency_test.go diff --git a/compilers/compilers.go b/compilers/compilers.go index dc8401b2..c4b98e8f 100644 --- a/compilers/compilers.go +++ b/compilers/compilers.go @@ -35,6 +35,11 @@ type Options struct { // no package-level mutable state, no writes to stderr; spec problems are // returned as ir.Diagnostic values and the error return is reserved for // I/O-level and programmer errors. +// +// Purity is also the concurrency contract. One Compiler value must accept +// overlapping Compile calls, which is what lets a caller share a single engine +// across goroutines; a Compiler that memoizes into package state would break +// that caller's guarantee without changing this signature. type Compiler interface { Formats() []SourceFormat Compile(ctx context.Context, sources []Source, opts Options) (*ir.Document, []ir.Diagnostic, error) @@ -43,6 +48,11 @@ type Compiler interface { // Registry maps source formats to compilers. It is a plain instance — there is // no package-level default and no init()-time self-registration; the engine // composes its registry explicitly. +// +// Concurrent Lookup is safe once registration is complete. Register is not: it +// writes an unsynchronized map, so every Register must happen before the first +// concurrent Lookup. Compose a Registry fully before publishing it, the way +// engine.NewWith registers into a fresh one and only then wraps it. type Registry struct { byFormat map[SourceFormat]Compiler } diff --git a/engine/concurrency_test.go b/engine/concurrency_test.go new file mode 100644 index 00000000..8b5fffdb --- /dev/null +++ b/engine/concurrency_test.go @@ -0,0 +1,141 @@ +package engine_test + +import ( + "context" + "encoding/json" + "fmt" + "path/filepath" + "sync" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/stretchr/testify/require" + + "github.com/dexpace/morphic/engine" +) + +// concurrencyCorpus is the capability corpus the compiler's own conformance +// suite drives. Reusing it here means the concurrent run covers every construct +// the compiler can lower rather than one hand-picked spec, so shared state +// hiding in a single lowering still gets exercised. +const concurrencyCorpus = "../testdata/conformance/openapi" + +// concurrentWorkers is the bounded fan-out: enough goroutines to overlap on any +// machine the suite runs on, few enough that a whole-corpus sweep per worker +// stays well inside the gate's per-package timeout even under -race. +const concurrentWorkers = 8 + +// TestEngine_ConcurrentRunSharesOneEngine drives the whole conformance corpus +// through a single *engine.Engine from several goroutines at once and requires +// every document to be byte-identical to the one an unshared Engine produced. +// One Engine, built outside the goroutines, is the whole point: a test that +// constructs an Engine per goroutine shares nothing and proves nothing. +// +// Two properties are pinned, and only the first is the detector's. A data race +// is not the only way concurrency corrupts output — a cache keyed on the wrong +// thing can be perfectly synchronized and still hand one caller another's +// answer — so the documents are compared, not merely produced, and the baseline +// is built with a *fresh* Engine per spec: a baseline drawn from the shared +// Engine would carry the same corruption and cancel it out. Matching it also +// re-checks determinism (CLAUDE.md invariant 7). Run under -race for the first +// property; the comparison holds either way. +func TestEngine_ConcurrentRunSharesOneEngine(t *testing.T) { + t.Parallel() + + specs := corpusSpecs(t) + ctx := t.Context() + + // The baseline: each spec compiled sequentially through an Engine of its own, + // so nothing at all is shared and nothing can carry over between compiles. + want := make([]string, len(specs)) + for i, spec := range specs { + fresh, err := engine.New() + require.NoError(t, err) + doc, err := compileJSON(ctx, fresh, spec) + require.NoError(t, err) + require.NotEmpty(t, doc, "baseline compile of %s produced nothing", spec) + want[i] = doc + } + + // A comparison against a degenerate baseline would pass for the wrong reason, + // so require the corpus to have produced a distinct document per spec. + distinct := make(map[string]struct{}, len(want)) + for _, doc := range want { + distinct[doc] = struct{}{} + } + require.Equal(t, len(specs), len(distinct), "baseline documents must all differ") + + eng, err := engine.New() + require.NoError(t, err) + + got := make([]workerRun, concurrentWorkers) + var wg sync.WaitGroup + for w := range concurrentWorkers { + wg.Add(1) + go func() { + defer wg.Done() + got[w] = runCorpus(ctx, eng, specs, w) + }() + } + wg.Wait() + + for w, run := range got { + require.NoError(t, run.err, "worker %d", w) + require.Len(t, run.docs, len(specs), "worker %d did not finish the corpus", w) + for i, spec := range specs { + if diff := cmp.Diff(want[i], run.docs[i]); diff != "" { + t.Errorf("worker %d compiled %s differently (-unshared +shared):\n%s", + w, filepath.Base(spec), diff) + } + } + } +} + +// workerRun is one goroutine's sweep of the corpus: the marshalled document per +// spec, in the corpus order, or the first failure it hit. +type workerRun struct { + docs []string + err error +} + +// runCorpus compiles every spec through eng, starting at offset so that workers +// are on different specs at any instant and overlapping calls exercise +// different lowerings rather than all crowding one. +func runCorpus(ctx context.Context, eng *engine.Engine, specs []string, offset int) workerRun { + docs := make([]string, len(specs)) + for i := range specs { + j := (offset + i) % len(specs) + doc, err := compileJSON(ctx, eng, specs[j]) + if err != nil { + return workerRun{err: err} + } + docs[j] = doc + } + return workerRun{docs: docs} +} + +// compileJSON runs one spec through eng and marshals the document. It returns +// errors rather than calling t.Fatal because it runs off the test goroutine, +// where require's FailNow is not legal. +func compileJSON(ctx context.Context, eng *engine.Engine, path string) (string, error) { + res, err := eng.Run(ctx, path, engine.RunOptions{}) + if err != nil { + return "", fmt.Errorf("run %s: %w", path, err) + } + if res.Document == nil { + return "", fmt.Errorf("run %s: nil document", path) + } + raw, err := json.Marshal(res.Document) + if err != nil { + return "", fmt.Errorf("marshal %s: %w", path, err) + } + return string(raw), nil +} + +func corpusSpecs(t *testing.T) []string { + t.Helper() + specs, err := filepath.Glob(filepath.Join(concurrencyCorpus, "*.yaml")) + require.NoError(t, err) + require.NotEmpty(t, specs, "no specs under %s", concurrencyCorpus) + return specs +} diff --git a/engine/engine.go b/engine/engine.go index 2885cf07..4a1c2339 100644 --- a/engine/engine.go +++ b/engine/engine.go @@ -29,6 +29,15 @@ type Result struct { // Engine orchestrates the sniff → compiler → passes pipeline over a registry of // compilers. +// +// An Engine is safe for concurrent use by multiple goroutines, and concurrent +// runs over one spec yield identical documents rather than merely uncorrupted +// ones. NewWith finishes writing the registry before the Engine exists and Run +// only reads it; Run keeps nothing between calls. The rest of the guarantee is +// compilers.Compiler's purity requirement — a compiler holding package-level +// mutable state would break it, which is why that requirement is part of the +// contract and not advice. TestEngine_ConcurrentRunSharesOneEngine pins both +// properties, the first only under -race. type Engine struct { registry *compilers.Registry } @@ -60,6 +69,10 @@ func NewWith(fronts ...compilers.Compiler) (*Engine, error) { // format, dispatch to the matching compiler, and — unless disabled — append the // validate pass's diagnostics. The Go error return is reserved for I/O and // programmer errors; spec problems surface as diagnostics in the Result. +// +// Calls on one Engine may overlap. They share only the read-only registry, and +// each call owns the document it returns — which is what makes appending the +// validate pass's diagnostics into that document safe. func (e *Engine) Run(ctx context.Context, specPath string, opts RunOptions) (*Result, error) { data, err := os.ReadFile(specPath) if err != nil { diff --git a/pass/validate.go b/pass/validate.go index c510b47c..cb873dcf 100644 --- a/pass/validate.go +++ b/pass/validate.go @@ -17,6 +17,10 @@ const maxGroupDepth = 128 // the diagnostics it finds, most-structural first. It is pure: it never mutates // doc and holds no package-level state. An empty result means the document is // internally consistent for every rule this pass enforces. +// +// Purity makes concurrent calls safe, including several over one document — but +// Validate takes no lock, so nothing may be mutating that document meanwhile. +// engine.Run satisfies this by validating a document no other run can reach. func Validate(doc *ir.Document) []ir.Diagnostic { if doc == nil { return nil From 20007a20991da67e6a92dcdc72ba6b29695a6770 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 13 Aug 2026 22:49:20 +0300 Subject: [PATCH 2/2] test(engine): say a worker never compiled a spec, not that it differed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A worker that stopped partway through left zero values in its results, and the comparison reported those as documents compiled differently — which sends a reader looking for a corrupted lowering when nothing lowered at all. Planting an early break made it print that for 74 of 77 specs. The loop now names the case and says so. The length check above it stays: it cannot fail while runCorpus returns either an error or a full-length slice, and it is what keeps the indexing below from panicking should that ever change. Its message no longer claims to be about finishing the corpus, which is the check that was just added. The Engine doc said the race property is pinned "only under -race" and left the reader to wonder whether anything runs it. #344 landed -race in the coverage step, so it does. --- engine/concurrency_test.go | 12 +++++++++++- engine/engine.go | 3 ++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/engine/concurrency_test.go b/engine/concurrency_test.go index 8b5fffdb..79ec6546 100644 --- a/engine/concurrency_test.go +++ b/engine/concurrency_test.go @@ -81,8 +81,18 @@ func TestEngine_ConcurrentRunSharesOneEngine(t *testing.T) { for w, run := range got { require.NoError(t, run.err, "worker %d", w) - require.Len(t, run.docs, len(specs), "worker %d did not finish the corpus", w) + // Bounds for the indexing below. It cannot fail while runCorpus returns + // either an error or a full-length slice, and it is here so that a later + // change to that shape is a failed assertion rather than a panic. + require.Len(t, run.docs, len(specs), "worker %d returned %d docs", w, len(run.docs)) for i, spec := range specs { + // A worker that stopped early leaves zero values behind, which the + // diff below would report as a document compiled differently. Say + // what actually happened instead. + if run.docs[i] == "" { + t.Errorf("worker %d never compiled %s", w, filepath.Base(spec)) + continue + } if diff := cmp.Diff(want[i], run.docs[i]); diff != "" { t.Errorf("worker %d compiled %s differently (-unshared +shared):\n%s", w, filepath.Base(spec), diff) diff --git a/engine/engine.go b/engine/engine.go index 9a27d04b..68e3d631 100644 --- a/engine/engine.go +++ b/engine/engine.go @@ -55,7 +55,8 @@ type Result struct { // compilers.Compiler's purity requirement — a compiler holding package-level // mutable state would break it, which is why that requirement is part of the // contract and not advice. TestEngine_ConcurrentRunSharesOneEngine pins both -// properties, the first only under -race. +// properties: the second on every run, the first only under -race, which the +// gate's coverage step passes. type Engine struct { registry *compilers.Registry }