fix(compilers/openapi): resolve aliases and merge keys in the cycle pre-scan - #94
Merged
OmarAlJarrah merged 3 commits intoJul 27, 2026
Merged
Conversation
…re-scan The pre-parse cycle scan reads the raw yaml.Node tree, while speakeasy's resolver reads the decoded tree where aliases (keys and values) are already dereferenced and `<<` merge keys are already expanded. Wherever the scan's node model was narrower than the decoder's, a genuine $ref cycle could slip past it and reach the resolver, which crashes the process with an unrecoverable stack overflow. Add mappingPairs, a helper that walks a mapping the way the decoder sees it: alias keys and values dereferenced, `<<` merge keys expanded, with the same key-precedence rules yaml.v3 applies. Route pureRefTarget, childByToken, and the schema-ref walk through it, and dereference schema nodes themselves so an alias standing in for a whole schema is followed too. Add contentSchema to the sub-schema key set, the one JSONSchema-typed field of oas3.Schema the existing sets missed. Following alias edges structurally also means the same subtree can now be reached through more than one path, so the ref-collection walk needs visited-node sets to keep it linear instead of letting a chained-alias document blow up exponentially. The walk is split across four functions — walkOutsideSchema, walkSchema, walkSchemaMap, and walkSchemaList — and each gets its own set. A single shared set per two of them is not safe: an anchored $ref node can legally be reused once where its own value is treated as a schema (walkSchema) and once where its values are treated as a name-to- schema map (walkSchemaMap), and letting the first role mark the node "seen" made the second role skip it — silently dropping the very $ref node the cycle chain needed and letting the resolver recurse unbounded. Each walk function keeps its own set instead, so a node is only ever skipped on a repeat visit in the same role. Add reproducers for all six shapes plus legal-document controls so the scan doesn't start refusing valid aliases and merges, and seed the fuzz corpus with every fixture under testdata/openapi.
…rom the ref walk Expanding `<<` merge keys made the cycle pre-scan super-linear. Every read of a mapping recomputed the full merge closure from scratch, and the schema walk read each node twice, so a merge chain cost O(n) per expansion and O(n) expansions: a 3200-line document took 41 seconds. The scan exists to keep a degenerate spec from crashing the compiler, so turning that crash into a hang is no fix. Reads of the raw node tree now go through a nodeView, which owns the decoder-faithful view of a mapping and memoizes each expansion for the lifetime of one scan. Only complete expansions are cached: one truncated by a merge cycle or by the depth cap is missing pairs that another entry point would supply, and caching it would let traversal order decide whether a $ref is found. The same 3200-line document now scans in 90ms. Two correctness fixes alongside it: - Merge-key detection now matches yaml.v3's own isMerge. The key is examined undereferenced and its tag is checked, so a quoted '<<' (tag !!str) and an alias standing in for the key are ordinary keys again. Both were being expanded as merges, which reported a cyclic $ref in documents that parse cleanly and would never have reached the resolver. - The ref-collection walk is iterative. Resolving aliases means one node is reachable from many parents, so the walk needs memoization to stay linear — but memoization and a recursion depth cap are unsound together: a node first reached near the cap has its descent truncated and is then skipped when a shallow path reaches it again, silently dropping the refs beneath it. A worklist has no stack to bound, so the cap is gone and each (node, role) pair is visited exactly once. The per-role visited sets are now one array indexed by role rather than a field each, so adding a role cannot leave a set nil.
The mapping view added for alias and merge-key resolution reused maxCycleDepth (10000) as its expansion bound. Expanding a `<<` chain re-materializes every pair the levels below it contributed, so that bound let a legal document cost far more than the document itself: a 180 KB spec with a 6000-level merge chain retained 2.2 GB, and one whose schemas were ordered deepest-first took 29s where the parser the scan protects handles the same input in 20ms. Two bounds replace it. maxMergeDepth (64) caps how deep a merge chain the view follows, which is what makes an over-deep chain cheap to stop expanding rather than expensive to expand — real specs merge one or two levels. maxCachedPairs caps what the expansion cache retains, so the scan holds a stated ~50 MB at worst whatever it is given. The same two documents now cost 13 MB and 0.77s, both scaling linearly with source size. Truncation is per node, not per scan. An earlier form of this latched a flag that stopped all further expansion, which would have let a spec disable its own cycle protection by carrying one over-deep chain ahead of a real cycle. A node that hits the bound now simply expands no further; every other mapping still expands in full, and refCycles reports the incompleteness as an openapi/cycle-scan-failed warning instead of passing the scan off as clean. Dropping pairs can only make a chain terminate early or a pointer dangle, never invent an edge, so a cycle found despite a truncation is still real and is still reported as the error. An expansion entered at the top level is memoized even when truncated: with nothing in flight around it, it is a deterministic function of the node alone, so caching it is sound and keeps a truncated chain from re-expanding once per node that references it. isMergeKey now applies speakeasy's yml.IsMergeKey test rather than yaml.v3's isMerge. Speakeasy's marshaller is what reads these documents, and the two disagree in both directions — yaml.v3 is laxer about the tag and honors only the last `<<` in a mapping, where speakeasy merges every one. Neither difference is reachable from a parsed document, but the comments pointed a future dependency bump at the wrong model to re-verify against. The ref-collection walk's dispatch gains an explicit roleSchemaList case and a panicking default, so a role added without a case fails loudly instead of being walked as whichever kind of node the switch fell through to. Behavior change: a spec whose merge chains nest deeper than 64 levels now carries an openapi/cycle-scan-failed warning. It still compiles — the warning is never a refusal — and nothing in the conformance corpus or golden set reaches the bound.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The pre-parse cycle scan reads the raw
yaml.Nodetree, while speakeasy's unmarshaller reads aresolved one — aliases (keys and values alike) dereferenced,
<<merge keys expanded. Whereverthe scan's node model was narrower than that, a genuine
$refcycle slipped past it and reached thereference resolver, which dies with
fatal error: stack overflow. That fault is unrecoverable, sothe process crashes instead of emitting the
openapi/cyclic-refdiagnostic the scan exists toproduce.
Six inputs reproduced the crash on
main. All are parseable documents:$refwhose value is a YAML aliaspureRefTargetrequired the value node to be a scalar$refundercontentSchemacontentSchemawas absent from the sub-schema key set$refkey that is itself an aliasValue$refpulled in by a<<merge key$refkey at allKind != MappingNode$refnode reused in two schema positionsThe change
A single resolver-faithful view of the tree. Every read of a mapping in the scan now goes
through
nodeView, which returns a mapping's effective pairs the way speakeasy's unmarshaller seesthem: alias keys and values dereferenced,
<<merge keys expanded, with its precedence rules(explicit over merged, earlier merge source over later).
pureRefTarget,childByToken, and theschema walk all route through it, and schema nodes are dereferenced at entry so an alias standing in
for a whole schema is followed.
Merge-key detection mirrors speakeasy's
yml.IsMergeKey, which its marshaller applies to everymapping it unmarshals via
yml.ResolveMergeKeys. The key is examined undereferenced and itsresolved tag is checked: an alias standing in for the key is not a scalar, and a quoted
'<<'resolves to
!!str. Speakeasy treats both as ordinary keys, so expanding them would invent pairs itnever sees and refuse a document that parses cleanly.
Worth recording, because it is what a future dependency bump has to re-verify against: yaml.v3's own
isMergeis not the right model even though it reads the same syntax. It is laxer about the tagand honors only the last
<<in a mapping, where speakeasy merges every one. Neither difference isreachable from a parsed document — yaml.v3 resolves plain, non-specific, and explicitly tagged
<<scalars alike to
!!merge— but the scan has to agree with speakeasy, not with yaml.v3.contentSchemajoinssubSchemaObjectKeys. I cross-checked the key sets field-by-field againstevery
*JSONSchema[Referenceable]-typed field ofoas3.Schemaat speakeasy v1.24.0; that was theonly one missing. The key-set doc comment now records the mapping — including the two entries
(
additionalItems,definitions) that are real JSON Schema keywords the library does not type — soa dependency bump has an explicit thing to re-verify.
The ref-collection walk is iterative, with one visited set per role. Following alias edges means
the same subtree is now reachable by several paths, so the walk needs memoization or a chained-alias
document makes it exponential — trading a crash for a hang.
Each of the four walk roles gets its own visited set rather than sharing one: an anchored
$refnodecan legally be reused once where the node itself is a schema and once where its values are a
name→schema map, and a shared set let the first role mark it seen so the second skipped it — dropping
the very node the cycle chain needed. That is the sixth row above, found while reviewing the first
draft of this fix. The dispatch over roles is exhaustive, with a panicking
default, so a role addedwithout a case fails loudly rather than being walked as whichever kind of node the switch fell
through to.
The walk is a worklist rather than a recursion because memoization and a recursion depth cap are
unsound together: a node first reached near the cap has its descent truncated and is then skipped
when a shallow path reaches it again, silently dropping every ref beneath it. A worklist has no stack
to bound, so the cap is gone and the visited sets alone bound the walk — each
(node, role)pair isenqueued exactly once. Children are pushed in reverse so the collection order stays the depth-first
pre-order a recursive walk produced, keeping the reported cycle stable for a document with more than
one.
Expansion is cached, and both the expansion and the cache are explicitly bounded. Expanding a
<<chain re-materializes every pair the levels below it contributed, so an unbounded expansion letsa legal document cost far more than the document: without a cache a 3200-line spec took 41s, and with
one but no depth bound tighter than the walk's, a 180 KB spec with a 6000-level merge chain retained
2.2 GB while a variant ordered deepest-first took 29s. Both are the same failure this scan exists to
prevent, arriving as a hang or an OOM instead of a crash.
Two bounds keep it stated rather than open-ended:
maxMergeDepth(64) caps how deep a merge chain the view follows. Expanding a chain of depth dcosts O(d²) and retains as much, so keeping d small is what makes an over-deep chain cheap to
stop expanding rather than expensive to expand. Hand-written specs merge one or two levels and no
generator emits more.
maxCachedPairscaps what the cache retains — ~50 MB at worst, whatever the input. Declining tocache costs a recomputation and nothing else, which is what lets the budget be a hard limit rather
than a heuristic.
The same two documents now cost 13 MB and 0.77s, both scaling linearly with source size. For
reference, speakeasy itself is super-linear on this shape — it re-flattens each merge chain per node
it unmarshals, taking 14s on a 135 KB document — so the scan is no longer close to the dominant cost
(0.3–7% of
Compileon these inputs).Truncation is per node, not per scan. A node that hits the depth bound expands no further; every
other mapping in the document still expands in full. That distinction is load-bearing: a bound that
switched the whole view off would let a spec disable its own crash protection by carrying one
over-deep merge chain ahead of a real cycle. Dropping pairs can only make a chain terminate early or
a pointer dangle — never invent an edge — so a cycle found despite a truncation is still real and is
still reported as the error, and a clean result on a truncated scan is only "no cycle found in what
could be expanded", which
refCyclessays out loud as anopenapi/cycle-scan-failedwarning.Only expansions that are reproducible are memoized: any complete one, and any entered at the top
level, which with nothing in flight around it is a deterministic function of the node alone. A
truncation reached from inside a deeper expansion is not — how much of the chain below survived
depends on where the walk came in — and caching that would let one traversal order lose a
$refanother would find.
walkAnchorsis deliberately untouched: it must not follow alias edges structurally, since that isexactly how it detects a recursive anchor.
Behavior change
A spec whose
<<merge chains nest deeper than 64 levels now carries anopenapi/cycle-scan-failedwarning. It still compiles — the warning is never a refusal, and speakeasy expands such chains fine —
it only states that the pre-parse guarantee is incomplete for that source. Nothing in the conformance
corpus or the golden set reaches the bound.
Test plan
testdata/openapi/, one per shape, registered in thecycleReproducerstable so both the detector-level test and the full-Compiletest cover each.A green run of
TestCompile_CyclicSpecDoesNotCrashis itself the proof of the fix: a fatal stackoverflow would take the test binary with it.
schema reuse, a legal
<<merge into a concrete schema, a legal non-cycliccontentSchema, andan alias-valued
$refwhose chain terminates. Each must lower cleanly with no cyclic-refdiagnostic.
TestDetectCycles_NonMergeKeyShapesAreCleanadds the resolver-fidelity pair — a quoted'<<'and an alias-valued key — which an over-eager merge expansion would refuse.TestIsMergeKey_MatchesResolvertables the tag and kind rules against speakeasy's, andTestIsMergeKey_AgreesWithParsedTagsbacks that strictness by taking tags from real parses ratherthan asserting them — showing no parsed document can reach the shapes the table rejects.
a key aliasing a nil target, non-scalar keys, duplicate keys, merge and merge-sequence precedence,
a non-mapping merge value, and the depth bound from both sides (a chain exactly at it expands in
full; one past it stops and records why).
TestNodeView_CachesOnlyReproducibleExpansionspins what may and may not be memoized across allfour cases, and
TestNodeView_MemoizeRespectsPairBudgetpins the cache ceiling including that adropped entry still reads correctly.
TestDetectCycles_TruncationDoesNotDisableTheRestOfTheScanandTestNodeView_TruncationIsPerNodepin the non-contagious rule end to end and at the unit level: a cycle declared after an over-deep
chain is still caught, and caught as the error rather than reported as the warning.
TestCompile_MergeChainPastBoundStillCompilespins that the warning never costs a compile.TestRefScanCollect_VisitsEachNodeOncePerRoledrives one anchored$refnodeinto all three schema roles and asserts it is collected once but entered in each role;
TestRefScanCollect_DeepNestingIsNotTruncatednests a$refpast the former depth cap and requiresit still be collected;
TestRefScanCollect_UnhandledRolePanicspins the exhaustive dispatch.chained-alias fan-out document (exponential without the visited sets), a merge chain at the depth
bound (must stay clean and cached), and a 1600-level chain with every level a schema and the
schemas ordered deepest-first — the shape that took 29s before the bound, which must now finish
fast and report the warning rather than claim to be clean.
seedCorpusnow also seeds everytestdata/openapi/*.yaml, so the degenerate shapes are mutationstarting points for
FuzzCompilerather than fixtures the fuzzer never sees.gofmt -l .,go vet ./...,golangci-lint run(0 issues),go test ./..., and./scripts/check-coverage.sh(100.0% total, 100.0% every package) all pass.Closes #26.