fix: release the reference lock before resolving its pointer - #230
Merged
TristanSpeakEasy merged 8 commits intoAug 6, 2026
Merged
Conversation
Reference.resolve held the reference's own cacheMutex write lock across the
call to references.Resolve. That call navigates the document, and navigating
into a reference calls GetObject, which takes a read lock on that reference.
sync.RWMutex is not reentrant, so a $ref whose JSON pointer passes through the
reference being resolved blocked forever on a lock its own goroutine held.
A pointer reaches the in-flight reference either directly, when its own prefix
names it:
paths:
/a: {$ref: '#/paths/~1a/t'}
or through GetObject's cache delegation, when an already-resolved reference
forwards to the one currently resolving:
paths:
/a: {$ref: '#/paths/~1b'}
/b: {$ref: '#/paths/~1a/t'}
Both hang the process. Neither is caught by resolveObjectWithTracking, whose
reference chain is only extended once a hop completes, so a hop that re-enters
itself is never compared against it.
The same lock scope caused a second failure. When a reference resolved to
itself -- reachable because GetJSONPointer trims the pointer, so
'#/paths/~1a ' names /a -- the resolution cache was left pointing at its own
reference, and GetObject's delegation to
referenceResolutionCache.Object.GetObject() recursed until the goroutine stack
was exhausted.
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, which is wasted effort rather than a correctness problem: the
published result is whichever completes first, and every caller returns it.
With the lock released, both shapes resolve to the errors they should always
have produced -- an unresolved reference, or "circular reference detected" from
the existing tracker.
TristanSpeakEasy
requested changes
Aug 2, 2026
TristanSpeakEasy
left a comment
TristanSpeakEasy
left a comment
Member
There was a problem hiding this comment.
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.Objectequal to the original reference.
Requesting changes for the resulting fatal GetObject recursion called out inline.
Releasing the reference lock keeps resolution from deadlocking, but it does not change what gets published. A $ref whose pointer names a reference already in the chain still leaves that reference holding a resolution cache that points back into the chain, and GetObject followed it by recursing into the next reference's GetObject. Walking a cycle that way exhausts the goroutine stack and aborts the process. Two shapes reach it. A reference can resolve to itself, and two references can resolve to each other -- the tracker only reports the cycle on the hop after both caches are published, so neither one is a self-reference. A pointer-identity check against the reference being resolved would catch the first and miss the second. Walk the chain iteratively instead, tracking what has been seen and returning nil once it repeats. That covers a cycle of any length, and it leaves the resolution errors and the published cache exactly as they were. Also skip the parent links when a reference resolves to itself, so GetParent and GetTopLevelParent do not loop for anyone walking them. The existing cases now assert the specific error they produce rather than just that one occurred, and every case checks GetObject afterwards, which is where the crash actually lived.
TristanSpeakEasy
requested changes
Aug 2, 2026
TristanSpeakEasy
left a comment
TristanSpeakEasy
left a comment
Member
There was a problem hiding this comment.
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.
The previous guard only covered a reference that resolved to itself. With /a and /b resolving to each other, the second hop has ref == b and nextRef == a, so it passed the guard and published a.parent = b alongside b.parent = a. GetObject no longer walks that, but GetParent and GetTopLevelParent are public and a caller walking them still loops. The absolute references the tracker already collects cannot answer this: a pointer can name a reference that is already in the chain, so one reference value can appear under two different absolute references. Track the reference values alongside them and skip the links for any value the chain has already been through. The cycle is still reported by the recursive call, so nothing that resolves cleanly loses its links. The two-reference case now asserts that both the parent chain and the top-level parent terminate.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
The deferred unlock inside the conditional, paired with a bare unlock on the branch below it, was correct but easy to break: every future edit to the block has to reason about which of the two paths releases the lock. Read the cache under the lock, release it, then branch on the copy. Same behaviour, one unlock, and it matches how the read-lock check above it already reads.
TristanSpeakEasy
requested changes
Aug 3, 2026
TristanSpeakEasy
left a comment
TristanSpeakEasy
left a comment
Member
There was a problem hiding this comment.
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.
The per-call chain only knew about hops made by one Resolve. Each member of a cycle can be resolved by its own call: resolving /a leaves b.parent = a, and resolving /b afterwards starts a fresh chain, sees /a as new, and adds a.parent = b on top of it. ResolveAllReferences never hit this because it skips references already marked resolved, but Reference.Resolve has no such guard. Ask the links instead. A reference is unsafe to parent to another when it is already reachable from it, which holds across calls because the links are the record that survives them. That subsumes what the chain covered, so the extra parameter goes away and the signature returns to what it was. TestResolve_SeparateCallsOverCycle resolves both members of a cycle in separate calls and walks their links; without the guard it fails on /a: parent links cycle.
TristanSpeakEasy
requested changes
Aug 4, 2026
TristanSpeakEasy
left a comment
TristanSpeakEasy
left a comment
Member
There was a problem hiding this comment.
Re-reviewed the latest follow-up. The ancestry-based guard fixes the previously reported sequential public-Resolve case, including reverse call order and three-member cycles.
Validation:
- Focused cycle, separate-call, chain, and parent-link tests: passed.
go test -race -count=1 ./openapi ./references ./jsonpointer: passed.go test -count=1 ./...: passed.- Reverse-order and three-separate-call probe: passed.
- Concurrent two-member
Resolveprobe under-race: failed with confirmed races onparentandtopLevelParent.
The ancestry check and parent publication are still not atomic across concurrent Resolve calls, called out inline. This review supersedes my review on 9f9c8a6a.
Member
Suggested regression matrix before the next re-reviewTo keep the fixes from moving the failure between resolution caches, Resolution and cache cycles
For every invalid case, assert:
Public
|
Deciding whether an edge is safe means reading links that belong to other references, so the check and the write that follows it have to be one operation. They were not: two resolvers running concurrently could each walk the ancestry, each see nothing, and then publish opposite edges, rebuilding the cycle the check exists to prevent. Reading ref.topLevelParent and walking GetParent also raced against SetParent and SetTopLevelParent, which the race detector reports once both members of a cycle are resolved at the same time. Per-reference locks cannot cover this, because the state being checked spans references. Guard the parent and topLevelParent fields of every Reference with one lock instead, and move the check and both writes into a single critical section under it. The accessors take it too, so the links are safe to read while another goroutine resolves. Resolution itself is unaffected: the links are only touched once per hop, against work that parses and navigates documents. Tests cover the matrix these fixes span: one, two and three node cycles; every member resolved separately, in both orders; the links a valid chain is expected to leave, asserted as exact pointers and unchanged by resolving twice; and two barrier-synchronized concurrent cases, one racing both members of a cycle and one racing many resolvers of the same valid reference.
Contributor
Author
|
Worked through the matrix — it is all on Resolution and cache cycles: direct pointer through itself in path, component path-item and webhook forms, one-node, two-node and three-node cycles, plus a three-hop chain that resolves. Every invalid case asserts the specific circular/unresolved error rather than accepting any error, runs under a 30s bound, and then checks Public Concurrency: barrier-synchronized, as you said, since parallel tests do not overlap reliably enough. One test races both members of a cycle, one races 16 resolvers of the same valid reference and checks every caller observes the object. Both leave the parent chains walkable. The missing regression you called out was the real one. It reproduced the races on Not taken as its own case: resolving the same valid reference repeatedly is covered sequentially by the valid-chain test and concurrently by the 16-resolver test. All three of your validation commands are clean, plus |
TristanSpeakEasy
approved these changes
Aug 5, 2026
TristanSpeakEasy
left a comment
TristanSpeakEasy
left a comment
Member
There was a problem hiding this comment.
Re-reviewed the graph-wide parent-link synchronization at dc2a4df.
The shared lock makes the ancestry check and both parent-link writes atomic, synchronizes the public parent accessors, and does not introduce a lock-order inversion with reference cache resolution. The expanded tests cover the full regression matrix: one-/two-/three-node cycles, separate public Resolve calls in both orders, exact valid-chain parent pointers, concurrent opposite cycle members, and concurrent same-reference resolution.
Validation:
go test -race -count=20 -run 'TestResolve_ConcurrentCallsOverCycle|TestResolve_ConcurrentCallsSameReference|TestResolve_SeparateCallsOverCycle|TestResolve_ValidChainParentLinks' ./openapi: passed.go test -race -count=1 ./openapi ./references ./jsonpointer: passed.go test -count=1 ./...: passed.- Full diff whitespace check: passed.
- PR checks: successful (one check passed, one skipped, none failing/pending).
The remaining review observations are non-blocking test-strengthening or pre-existing API behavior, not regressions introduced by this PR. Approving; this review supersedes my earlier changes-requested reviews.
TristanSpeakEasy
enabled auto-merge (squash)
August 5, 2026 23:33
Contributor
📊 Test Coverage ReportCurrent Coverage: Coverage Change: ✅ No change Coverage by Package
📋 Detailed Coverage by Function (click to expand)
Generated by GitHub Actions |
Contributor
Author
|
Hey @TristanSpeakEasy |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
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
2 participants
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
A JSON pointer that passes through the reference it is resolving breaks three separate things: resolution deadlocks,
GetObjectrecurses until the stack is gone, and the parent links are left pointing in a loop. Each is fixed below.1. Resolution deadlocks
Reference.resolveheld the reference's owncacheMutexwrite lock across the call toreferences.Resolve(reference.go#L537-L557). That call navigates the document, and navigating into a reference callsGetObject, which takes a read lock on that reference (reference.go#L293).sync.RWMutexis not reentrant, so a$refwhose 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 withfatal error: all goroutines are asleep - deadlock!.An 83-byte document is enough:
ResolveAllReferencesnever returns.How the pointer reaches the in-flight reference
Directly, when the pointer's own prefix names it — the case above. The final
/tsegment is never evaluated; the walk deadlocks on the#/paths/~1aprefix.Through the cache delegation at reference.go#L299, where
GetObjectforwards toreferenceResolutionCache.Object.GetObject(). An already-resolved reference forwards into the one currently resolving:Resolving
/acompletes, then resolving/bnavigates to/a, which forwards straight back into/bwhile/b's write lock is held.Neither shape is caught by
resolveObjectWithTracking: itsreferenceChainis 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.
GetObjectrecurses through a cyclic resolution cacheIndependent 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'sGetObject. A loop there exhausts the goroutine stack and aborts the process:Two shapes reach it. A reference can resolve to itself, because
GetJSONPointertrims the pointer, so'#/paths/~1a '— one trailing space — names/a:Or two references can resolve to each other, where neither is a self-reference:
Both report the circular reference they should —
circular reference detected: test.yaml#/paths/~1a -> test.yaml#/paths/~1aand 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 callsGetObjecttakes 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.
GetObjectwalks 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 —resolveObjectWithTrackingreadsref.referenceResolutionCache.ResolvedDocumentas soon asresolvereturns a next reference, so an empty cache trades the overflow for a nil dereference.3. Parent links close the same loop
resolveObjectWithTrackingsetsSetParent/SetTopLevelParenton each hop before recursing, including the hop that closes a cycle. In the two-reference case that leavesa.parent = balongsideb.parent = a, so a caller walking the publicGetParentorGetTopLevelParentloops 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
referenceChainand 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_PointerTraversingItsOwnReferencecovers 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 callsGetObjectand walks the parent links, which is where the second and third defects live.TestGetObject_ChainWalkingpins 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.
GetObjectwalk: both cycle shapes abort the binary withfatal error: stack overflow./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 -fuzzwires worker stderr to/dev/null, so it only ever surfaced asEOF.Note: the schema resolver has the same defect, not fixed here
jsonschema/oas3resolves 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 onmainand 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:ResolveAllReferencesaborts the process withfatal error: stack overflowinresolveJSONSchemaWithTracking. 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
nilin place of the reference chain: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:
The parent links have the matching problem: resolution.go#L398-L399 sets them unconditionally, so the same probe shows
schema.parent == schemaandschema.topLevelParent == schemafrom the second hop on — the schema-side version of the parent-link fix in this PR.Worth a follow-up.
Summary by cubic
Prevents self-deadlocks, stack overflows, and parent-link cycles during $ref resolution. Resolution, GetObject, and parent-link access are now safe under cycles and concurrency; cyclic/self-refs return clear circular/unresolved errors instead of crashing.
Bug Fixes
cacheMutexwhile callingreferences.Resolve; re-check and publish under lock to avoid self-deadlocks when traversal hits the in-flight ref.GetObjectiteratively with cycle detection; return nil on loops instead of recursing.referenceParentMutex;GetParent/GetTopLevelParent/Set*use the same lock to prevent races, skip linking when the next ref is already an ancestor/self, and stop concurrent resolvers from rebuilding cycles.Refactors
Reference.resolve: read caches under lock, unlock, branch on copies; re-lock only to publish.Written for commit abd2b11. Summary will update on new commits.