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