diff --git a/compilers/compilers.go b/compilers/compilers.go index 62ef1bf..4cd2c21 100644 --- a/compilers/compilers.go +++ b/compilers/compilers.go @@ -60,6 +60,11 @@ type OptionSet struct { // 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. +// // Detect and DecodeOptions are what keep the layers above format-agnostic: a // compiler says what its own input looks like and what its own options are // called, so registering one is the whole of adding a format. Both are required @@ -102,6 +107,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. The zero value is a usable empty registry. +// +// 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 ordered []Compiler diff --git a/engine/concurrency_test.go b/engine/concurrency_test.go new file mode 100644 index 0000000..79ec654 --- /dev/null +++ b/engine/concurrency_test.go @@ -0,0 +1,151 @@ +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) + // 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) + } + } + } +} + +// 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 f89e85b..68e3d63 100644 --- a/engine/engine.go +++ b/engine/engine.go @@ -47,6 +47,16 @@ type Result struct { // Engine orchestrates the detect → 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 second on every run, the first only under -race, which the +// gate's coverage step passes. type Engine struct { registry *compilers.Registry } @@ -99,6 +109,10 @@ func NewWith(fronts ...compilers.Compiler) (*Engine, error) { // Result, a source no compiler can lower included. A caller that treats a Go // error as "the pipeline was invoked wrongly" therefore stays correct, which is // what lets the CLI keep its usage exit code for actual misuse. +// +// 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) { // Ahead of the read, because an engine that never went through a constructor // is the caller's mistake whatever the path turns out to say, and reporting diff --git a/pass/validate.go b/pass/validate.go index 5379335..ef71e5e 100644 --- a/pass/validate.go +++ b/pass/validate.go @@ -18,6 +18,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