fix: release the reference lock before resolving its pointer

[OmarAlJarrah]

Summary

A JSON pointer that passes through the reference it is resolving breaks three separate things: resolution deadlocks, GetObject recurses until the stack is gone, and the parent links are left pointing in a loop. Each is fixed below.

1. Resolution deadlocks

Reference.resolve held the reference's own cacheMutex write lock across the call to references.Resolve (reference.go#L537-L557). That call navigates the document, and navigating into a reference calls GetObject, which takes a read lock on that reference (reference.go#L293).

sync.RWMutex is not reentrant, so a $ref whose JSON pointer passes through the reference being resolved blocks forever on a lock its own goroutine holds. No concurrency is involved — this is a single-goroutine self-deadlock, and standalone it aborts with fatal error: all goroutines are asleep - deadlock!.

An 83-byte document is enough:

openapi: 3.1.0
info: {title: t, version: "1"}
paths:
  /a: {$ref: '#/paths/~1a/t'}

ResolveAllReferences never returns.

How the pointer reaches the in-flight reference

Directly, when the pointer's own prefix names it — the case above. The final /t segment is never evaluated; the walk deadlocks on the #/paths/~1a prefix.

Through the cache delegation at reference.go#L299, where GetObject forwards to referenceResolutionCache.Object.GetObject(). An already-resolved reference forwards into the one currently resolving:

paths:
  /a: {$ref: '#/paths/~1b'}
  /b: {$ref: '#/paths/~1a/t'}

Resolving /a completes, then resolving /b navigates to /a, which forwards straight back into /b while /b's write lock is held.

Neither shape is caught by resolveObjectWithTracking: its referenceChain is only extended once a hop completes, so a hop that re-enters itself is never compared against it.

The change. Take the write lock only for the double-check, release it while resolving, and re-acquire to publish the result — re-checking the cache in case another goroutine published first.

Concurrent resolution of the same reference may now duplicate work. The published result is whichever completes first and every caller returns it, so the resolved value is consistent; the cost is a second traversal, and the discarded result's entry in the root document's object cache.

2. GetObject recurses through a cyclic resolution cache

Independent of the lock, and not fixed by changing it. When a resolution chain closes a loop, the references involved are left holding caches that point back into it, and GetObject's delegation walked that by recursing into the next reference's GetObject. A loop there exhausts the goroutine stack and aborts the process:

runtime: goroutine stack exceeds 1000000000-byte limit
fatal error: stack overflow

Two shapes reach it. A reference can resolve to itself, because GetJSONPointer trims the pointer, so '#/paths/~1a ' — one trailing space — names /a:

paths:
  /a:
    $ref: '#/paths/~1a '
    get: {operationId: a, responses: {"200": {description: ok}}}

Or two references can resolve to each other, where neither is a self-reference:

paths:
  /a:
    $ref: '#/paths/~1b '
    get: {operationId: a, responses: {"200": {description: ok}}}
  /b:
    $ref: '#/paths/~1a '
    get: {operationId: b, responses: {"200": {description: ok}}}

Both report the circular reference they should — circular reference detected: test.yaml#/paths/~1a -> test.yaml#/paths/~1a and the two-hop equivalent — but the tracker only reaches that verdict on the hop after the caches are published, so the loop is already in place by the time it returns. Any consumer that then calls GetObject takes the process down.

Legitimate circular references are unaffected: there the last hop is never resolved, so its cache stays empty and the chain terminates.

The change. GetObject walks the chain iteratively, tracking what it has seen and returning nil once a reference repeats. That covers a cycle of any length and leaves the resolution errors and the published cache exactly as they were. Suppressing the publish instead is not viable — resolveObjectWithTracking reads ref.referenceResolutionCache.ResolvedDocument as soon as resolve returns a next reference, so an empty cache trades the overflow for a nil dereference.

3. Parent links close the same loop

resolveObjectWithTracking sets SetParent/SetTopLevelParent on each hop before recursing, including the hop that closes a cycle. In the two-reference case that leaves a.parent = b alongside b.parent = a, so a caller walking the public GetParent or GetTopLevelParent loops forever.

The absolute references the tracker already collects cannot answer this on their own: a pointer can name a reference that is already in the chain, so one reference value can appear under two different absolute references.

The change. Carry the reference values alongside referenceChain and skip the links for any value the chain has already been through. The cycle is still reported by the recursive call, and anything that resolves cleanly keeps its links.

Test plan

TestResolveAllReferences_PointerTraversingItsOwnReference covers seven shapes: direct prefix, prefix with a resolving pointer, the cache-delegation chain, the components spelling (#/components/pathItems/A/t), the webhooks spelling, the trimmed-pointer self-reference, and the two-reference cycle. Each asserts the specific error the document should produce, then calls GetObject and walks the parent links, which is where the second and third defects live.

TestGetObject_ChainWalking pins both ends of the chain walk: a three-hop chain still reaches its object, and a legitimate circular reference still reports no object.

The resolve runs in a goroutine with a 30-second bound, so a deadlock regression fails the test rather than hanging the suite.

• Without the lock change: the five deadlock shapes fail on the timeout.
• Without the GetObject walk: both cycle shapes abort the binary with fatal error: stack overflow.
• Without the parent-link tracking: the two-reference case fails on /a: parent links cycle.
• go test ./... — 44 packages, 0 failures.
• go test -race ./openapi/... ./references/... ./jsonpointer/... — clean.
• golangci-lint run ./openapi/... — 0 issues.

Found by fuzzing a downstream consumer, where the deadlock presented as workers dying with no diagnostic — go test -fuzz wires worker stderr to /dev/null, so it only ever surfaced as EOF.

Note: the schema resolver has the same defect, not fixed here

jsonschema/oas3 resolves schema references through its own tracker, and that one still recurses forever on a reference cycle. It is unchanged by this PR and reproduces identically on main and on this branch, so it is called out here rather than folded in.

It needs a schema whose entire body is a $ref — an alias — participating in a cycle:

components:
  schemas:
    A: {$ref: '#/components/schemas/B'}
    B: {$ref: '#/components/schemas/A'}

ResolveAllReferences aborts the process with fatal error: stack overflow in resolveJSONSchemaWithTracking. A bare self-alias (A: {$ref: '#/components/schemas/A'}) does the same.

Ordinary recursive schemas are unaffected — a schema referencing itself through a property, or two schemas referencing each other through properties, both resolve cleanly. Spelled with path items instead of schemas, the alias cycle above is already reported correctly as circular reference detected: test.yaml#/components/pathItems/A -> ... -> test.yaml#/components/pathItems/A, so this is an asymmetry between the two resolvers rather than a gap in cycle handling generally.

The cause is the cache short-circuit at resolution.go#L173-L177, which returns nil in place of the reference chain:

if s.referenceResolutionCache != nil {
    if s.referenceResolutionCache.Object != nil {
        return nil, nil, nil        // chain discarded
    }

Every cached hop hands an empty chain back to the recursion at resolution.go#L403, so the tracker resets and never matches. Instrumenting the recursion on the self-alias case shows the chain filling once and then staying empty against a schema pointer that never changes:

depth=1 len(chain)=0 schema=0x...b08 cacheSet=false
depth=2 len(chain)=1 schema=0x...b08 cacheSet=true
depth=3 len(chain)=0 schema=0x...b08 cacheSet=true
depth=4 len(chain)=0 schema=0x...b08 cacheSet=true
...

The parent links have the matching problem: resolution.go#L398-L399 sets them unconditionally, so the same probe shows schema.parent == schema and schema.topLevelParent == schema from the second hop on — the schema-side version of the parent-link fix in this PR.

Worth a follow-up.

---

Summary by cubic

Fixes self-deadlocks and stack overflows in $ref resolution by releasing the reference lock during resolve, walking cache chains iteratively with cycle detection, and preventing cyclic parent links using existing links. Cyclic/self-refs now return clear circular/unresolved errors without crashing.

• Bug Fixes

  • Release cacheMutex before calling references.Resolve, re-acquire only to publish; avoids sync.RWMutex self-deadlocks when traversal reaches the in-flight ref.
  • Make GetObject iterate through the resolution chain and track seen references; return nil on cycles instead of recursing.
  • Guard parent links with an isAncestor check over existing GetParent links; skip setting Parent/TopLevelParent when it would create a loop, including across separate Resolve calls.
  • Tests updated with timeouts and new cases for chain walking, cache delegation, components/webhooks, trimmed-pointer/two-reference cycles, and separate-call cycles; also assert parent links terminate.

• Refactors

  • Flattened the double-check in Reference.resolve: read caches under lock, unlock, then branch on copies; re-lock only to publish. Simplifies locking and matches the read-lock pattern above.

^{Written for commit f58f208. Summary will update on new commits.}

[cubic-dev-ai]

No issues found across 2 files

_{Re-trigger cubic}

[TristanSpeakEasy]

Reviewed the lock-scope change and its post-resolution cache state.

Validation:

• go test -count=1 -run TestResolveAllReferences_PointerTraversingItsOwnReference ./openapi: passed.
• go test -race -count=1 ./openapi ./references ./jsonpointer: passed.
• A focused probe confirmed that the trimmed-pointer case returns a circular-reference error but leaves referenceResolutionCache.Object equal to the original reference.

Requesting changes for the resulting fatal GetObject recursion called out inline.

[TristanSpeakEasy]

This still publishes a self-reference for the trimmed-pointer case. I verified after ResolveAllReferences returns circular reference detected that ref.referenceResolutionCache.Object == ref. The tracker sets circularErrorFound later, but it does not clear this cache entry, so any subsequent ref.GetObject() reaches line 299 and delegates straight back to ref.GetObject() until the process exhausts its goroutine stack. The new test only checks the resolution error and therefore misses the corrupt post-resolution state. Please avoid publishing a result whose object is this reference (or otherwise represent the circular state without a self-delegating cache), and add a regression that safely verifies GetObject() after the circular resolution attempt.

[OmarAlJarrah]

Confirmed and fixed in d02ba76, though not by suppressing the publish.

A pointer-identity check against the reference being resolved turned out to catch only the one-node case. Two references can close the loop between them without either being a self-reference:

paths:
  /a:
    $ref: '#/paths/~1b '
    get: {operationId: a, responses: {"200": {description: ok}}}
  /b:
    $ref: '#/paths/~1a '
    get: {operationId: b, responses: {"200": {description: ok}}}

The tracker reports circular reference detected: test.yaml#/paths/~1b -> test.yaml#/paths/~1a -> test.yaml#/paths/~1b, but only on the hop after both caches are published, so a.cache.Object == b and b.cache.Object == a and neither == r. GetObject then alternates between them until the stack goes.

Not publishing is also awkward here: resolveObjectWithTracking reads ref.referenceResolutionCache.ResolvedDocument right after resolve returns a next reference, so an empty cache turns the overflow into a nil dereference.

So GetObject now walks the chain iteratively with a seen set and returns nil once it repeats, which covers a cycle of any length and leaves the resolution errors and the published cache as they were. Legitimate circular references were already safe because the tracker stops before the last hop's cache is published, and there is now a test pinning that alongside a multi-hop chain that still resolves.

Both cycle shapes are regression tested through GetObject after resolution: with the walk reverted they abort the binary with fatal error: stack overflow. go test ./..., -race on ./openapi/... ./references/... ./jsonpointer/..., and golangci-lint are clean.

[TristanSpeakEasy]

Re-reviewed the follow-up commit. The previous fatal GetObject recursion is fixed by the iterative cycle-aware chain walk, and the strengthened tests cover both self-cycles and multi-reference cycles.

Validation:

• go test -count=1 -run 'TestResolveAllReferences_PointerTraversingItsOwnReference|TestGetObject_ChainWalking' ./openapi: passed.
• go test -race -count=1 ./openapi ./references ./jsonpointer: passed.
• go test -count=1 ./...: passed.
• Same-reference concurrent-resolution race probe: passed across 10 race-enabled runs.

A focused probe against the new two-reference case confirmed a remaining cyclic parent-link state, called out inline. This review supersedes my earlier request for changes on 3efdc1c.

[TristanSpeakEasy]

This guard fixes only the one-node self-cycle. In the new /a -> /b -> /a case, the second hop has ref == b, nextRef == a, and topLevel == a, so the guard passes and publishes a.parent = b, b.parent = a, and a.topLevelParent = a. I reproduced all three identities after ResolveAllReferences returned the expected circular-reference error. GetObject is now safe, but any consumer walking the public parent links can still loop forever, which is the state this comment says it prevents. Please avoid setting parent links when nextRef is already part of the current resolution chain (at minimum when nextRef == topLevel) and extend the two-reference regression to assert that parent/top-level-parent traversal cannot cycle.

[OmarAlJarrah]

Confirmed, and the guard was too narrow for the reason you give. Fixed in 1fe8ffc.

nextRef == topLevel would have caught this case, but not a chain that rejoins below the head: with /a -> /b -> /c -> /b, the third hop has nextRef == b and topLevel == a, so it would set b.parent = c on top of the existing c.parent = b. Parent links are between reference values, and the absolute references the tracker already collects can't stand in for them, since a pointer can name a reference that is already in the chain and make one value appear under two different absolute references.

So resolveObjectWithTracking now carries the reference values alongside referenceChain, and the links are skipped for any value the chain has already been through. The cycle is still reported by the recursive call, and anything that resolves cleanly keeps its links.

The two-reference case asserts both directions now: it walks GetParent to a terminus and checks GetTopLevelParent does not point at the reference itself. Reverting to the old guard fails it on /a: parent links cycle.

go test ./..., go test -race ./openapi/... ./references/... ./jsonpointer/... and golangci-lint run ./openapi/... are clean.

[cubic-dev-ai]

All reported issues were addressed across 3 files (changes from recent commits).

<sub>Tip: Review your code locally with the cubic CLI to iterate faster.

Fix all with cubic | Re-trigger cubic</sub>

[TristanSpeakEasy]

Re-reviewed the two follow-up commits. The identity-chain guard fixes parent links for a single ResolveAllReferences traversal, including cycles longer than two nodes, and the flattened cache double-check preserves the intended lock behavior.

Validation:

• Focused cycle, chain, and parent-link tests: passed.
• go test -race -count=1 ./openapi ./references ./jsonpointer: passed.
• go test -count=1 ./...: passed.
• Three-reference-cycle parent-link probe: passed.
• Same-reference concurrent-resolution probe: passed across 10 race-enabled runs.

A focused public-API re-entry probe confirmed the parent cycle can still be recreated across two separate Reference.Resolve calls, called out inline. This review supersedes my review on d02ba760.

[TristanSpeakEasy]

This identity chain only covers the current Resolve invocation, so the parent cycle can still be recreated across separate calls to the public API. With /a -> /b -> /a, a.Resolve(ctx, opts) leaves b.parent = a. Calling b.Resolve(ctx, opts) afterwards starts with a fresh resolvedChain; it sees a as new and adds a.parent = b plus a.topLevelParent = a. I reproduced all three identities (a.parent == b, b.parent == a, a.topLevelParent == a) on this head. ResolveAllReferences hides the issue because resolveAny skips references already marked resolved, but Reference.Resolve has no such guard. Please account for existing parent/top-level-parent links when deciding whether this edge is safe (or clear stale links when starting a root resolution), and add a regression that resolves both members separately before walking their parent links.

[OmarAlJarrah]

Confirmed, and fixed in f58f208. Reproduced all three identities on the previous head before changing anything.

The per-call chain was the wrong source of truth. Ancestry now comes from the links themselves:

if !isAncestor(ref, nextRef) && topLevel != nextRef {
    nextRef.SetParent(ref)
    nextRef.SetTopLevelParent(topLevel)
}

isAncestor walks GetParent from ref and reports whether nextRef is already reachable. That holds across calls because the links are the record that survives them, and it subsumes what the chain covered — within one call the links are set as each hop is taken, so ancestry and chain membership agree. The extra parameter is gone and the signature is back to what it was.

I went with reading the links rather than clearing them at the root of a resolution: a reference can legitimately carry links from an earlier chain that a caller already holds, and clearing on every root resolve would drop those.

The topLevel != nextRef half is belt and braces. Top-level parent is normally the head of the parent chain, so isAncestor already covers it, but the two fields are set independently through the public API and this keeps the self-link impossible either way.

TestResolve_SeparateCallsOverCycle resolves /a then /b in separate Resolve calls, walks both parent chains to a terminus, and asserts /a is not parented under /b. Unguarded it fails on /a: parent links cycle.

go test ./..., go test -race ./openapi/... ./references/... ./jsonpointer/... and golangci-lint run ./openapi/... are clean.