Skip to content

docs(engine): state and pin the concurrent-use guarantee - #332

Open
OmarAlJarrah wants to merge 1 commit into
mainfrom
docs/engine-concurrency-guarantee
Open

docs(engine): state and pin the concurrent-use guarantee#332
OmarAlJarrah wants to merge 1 commit into
mainfrom
docs/engine-concurrency-guarantee

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

Summary

One engine.Engine can serve Run calls from several goroutines at once, but nothing
said so and nothing pinned it. An embedder had no way to tell, so the safe move was to
over-construct — an engine per request — and a later change that parked a cache or a
scratch buffer on the Engine would have broken the property with a green build.

Rather than take the "pure, reentrant stages" rule on trust, I checked that the guarantee
actually holds before writing it down:

  • Every package-level var reachable from Run (the 20 packages in go list -deps ./engine) is a declaration-only table: keyword sets, reflect.Type values, sentinel
    errors. None is assigned or appended to anywhere after package init.
  • The codebase does use package-level function variables as test seams — newEngine,
    createOutput, renameOutput, harness.compile, reserializeJSON, encodeYAML
    and those are genuinely mutable package state; a test swapping one while another
    goroutine compiled would race. But all of them live in cmd/morphic,
    cmd/morphic-harness, internal/harness and ir/irtest, none of which is reachable
    from engine.Run. They do not narrow the guarantee.

So the guarantee is stated as broadly as it is true, and the one place it is genuinely
narrower is called out rather than papered over: compilers.Registry.Lookup is safe
concurrently only once registration has finished, because Register writes an
unsynchronized map. engine.NewWith satisfies that by construction — it fills a fresh
registry and only then wraps it — but an embedder registering a compiler after starting
goroutines would not.

Doc comments on engine.Engine, engine.Engine.Run, compilers.Compiler,
compilers.Registry and pass.Validate, plus one new test. No behaviour change.

This deliberately does not touch .github/workflows/gate.yml; adding -race to the gate
is a separate change. The new test is race-clean today, so it is covered the moment that
step lands, and until then it still earns its keep as a determinism check. For whoever
sizes that step: the full suite under -race is 49s wall-clock here, and the slowest
single package is internal/harness at 18.7s — well inside the coverage script's
-timeout 90s.

Test plan

engine/concurrency_test.go drives the whole 65-spec conformance corpus through a single
shared *engine.Engine from 8 goroutines, and requires every document to be
byte-identical to a baseline compiled with a fresh Engine per spec.

Two choices in it are load-bearing. It does not build an Engine per goroutine — that
shares nothing and proves nothing. And it does not stop at "no race reported": a data
race is only one way concurrency corrupts output, so the documents are compared, which
also re-checks the determinism invariant. The baseline uses unshared engines on purpose;
a baseline drawn from the shared engine would carry the same corruption as the concurrent
runs and cancel it out.

Clean run:

$ go test ./engine -race -run TestEngine_ConcurrentRunSharesOneEngine -count=1 -v
=== RUN   TestEngine_ConcurrentRunSharesOneEngine
=== PAUSE TestEngine_ConcurrentRunSharesOneEngine
=== CONT  TestEngine_ConcurrentRunSharesOneEngine
--- PASS: TestEngine_ConcurrentRunSharesOneEngine (2.84s)
PASS
ok  	github.com/dexpace/morphic/engine	4.242s

Whole repo, race detector on: go test ./... -race -count=1 exits 0 with zero
WARNING: DATA RACE occurrences.

Proof the test bites

Three deliberate defects, each planted, observed, and reverted.

  1. Per-instance state on the Engine (lastFormat field, assigned in Run) — the
    shape a memoizing Engine would take. The new test fails under -race:

    WARNING: DATA RACE
    Write at 0x00c00043c0f8 by goroutine 2193:
      github.com/dexpace/morphic/engine.(*Engine).Run()
    ...
        testing.go:1712: race detected during execution of test
    --- FAIL: TestEngine_ConcurrentRunSharesOneEngine
    

    The pre-existing engine tests all pass with this planted, because each builds its own
    Engine; sharing one is what catches it. Note also that the whole suite is green
    without -race, which is exactly why the detector step matters.

  2. A race-free wrong answer — a fully mutex-guarded memo on the Engine, keyed on
    source format instead of on the spec. Zero DATA RACE reports, and the test still
    fails, on the comparison:

    concurrency_test.go:86: worker 0 compiled allof-inheritance.yaml differently (-unshared +shared):
    concurrency_test.go:86: worker 0 compiled allof-inline-merge.yaml differently (-unshared +shared):
    

    This is the case a race-only test would wave through, and the reason the baseline is
    built from unshared engines.

  3. A degenerate baseline — every baseline entry compiled from the same spec, to
    confirm the anti-vacuity guard is not decorative:

    expected: 65
    actual  : 1
    Messages: baseline documents must all differ
    

Gate

gofmt, go vet, golangci-lint, go build and ./scripts/check-coverage.sh all pass;
coverage stays at 100%.

Closes #82

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

engine: document and pin the concurrent-use guarantee

1 participant