chore: detect circular dependency to avoid stack overflow#59
chore: detect circular dependency to avoid stack overflow#59leoparente wants to merge 1 commit intodevelopfrom
Conversation
leoparente
changed the title
|
Go test coverage
Total coverage: 76.7% |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d840d38bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Walk through all fields of the entity struct | ||
| for i := 0; i < elem.NumField(); i++ { | ||
| field := elem.Field(i) | ||
|
|
||
| // Handle pointer fields |
There was a problem hiding this comment.
The new cycle detector only recurses into pointer and slice fields, so references stored in maps or interfaces are skipped. That leaves a real gap for existing entity graphs: many entities carry CustomFields map[string]*CustomFieldValue (e.g., ASN in diode/ingester.go:35-50), and CustomFieldValue can hold CustomFieldObjectReference values that wrap another Entity and call ConvertToProtoMessage (see diode/ingester.go:2619-2666). A cycle like Device.CustomFields["x"].Value = &CustomFieldObjectReference{Object: device} will still recurse indefinitely during conversion even though detectCyclesInEntity runs. Consider walking map values (and interface/struct fields) so these entity-backed custom fields are included in the cycle check.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds cycle detection to the entity->proto conversion path to prevent infinite recursion/stack overflows caused by circular references during ingestion.
Changes:
- Changed
convertEntitiesToPrototo return([]*diodepb.Entity, error)and to fail fast on detected cycles. - Added a reflection-based recursive graph walk (
detectCycles*) to detect circular references before proto conversion. - Updated all
Ingestwrappers (gRPC, OTLP, dry-run) and tests to handle the new error-returning conversion API.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
diode/client.go |
Adds cycle detection + changes conversion helper signature; updates gRPC client Ingest to handle conversion errors. |
diode/client_test.go |
Updates existing conversion test for error handling; adds new circular reference detection tests. |
diode/dryrun.go |
Updates DryRunClient.Ingest to handle conversion errors. |
diode/otlp_client.go |
Updates OTLPClient.Ingest to handle conversion errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Get the pointer value of the entity using reflection | ||
| v := reflect.ValueOf(entity) | ||
| if v.Kind() != reflect.Ptr { | ||
| return nil | ||
| } | ||
| if v.IsNil() { | ||
| return nil | ||
| } | ||
|
|
||
| ptr := v.Pointer() | ||
|
|
||
| // Check if we've seen this entity in the current path (cycle detected) | ||
| if visited[ptr] { | ||
| return fmt.Errorf("circular reference detected: %s", strings.Join(path, " -> ")) | ||
| } | ||
|
|
||
| // Mark as visited | ||
| visited[ptr] = true | ||
|
|
||
| // Get the entity type name for error messages | ||
| elem := v.Elem() | ||
|
|
||
| // Only walk through struct fields | ||
| if elem.Kind() != reflect.Struct { | ||
| // Still need to unmark before returning | ||
| delete(visited, ptr) | ||
| return nil | ||
| } | ||
|
|
||
| typeName := elem.Type().Name() | ||
| newPath := append(path, typeName) | ||
|
|
||
| // Walk through all fields of the entity struct | ||
| for i := 0; i < elem.NumField(); i++ { | ||
| field := elem.Field(i) | ||
|
|
||
| // Handle pointer fields | ||
| if field.Kind() == reflect.Ptr && !field.IsNil() { | ||
| // Check if this field implements Entity interface | ||
| if field.CanInterface() { | ||
| fieldValue := field.Interface() | ||
| if _, ok := fieldValue.(Entity); ok { | ||
| if err := detectCyclesInEntity(fieldValue, visited, newPath); err != nil { | ||
| return err | ||
| } | ||
| } else { | ||
| // For non-Entity types, still recurse to check nested entities | ||
| if err := detectCyclesInEntity(fieldValue, visited, newPath); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Handle slice fields (e.g., []*Tag, []*VLAN) | ||
| if field.Kind() == reflect.Slice { | ||
| for j := 0; j < field.Len(); j++ { | ||
| item := field.Index(j) | ||
| if item.Kind() == reflect.Ptr && !item.IsNil() && item.CanInterface() { | ||
| if err := detectCyclesInEntity(item.Interface(), visited, newPath); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Unmark after visiting all children (allow same entity in different branches) | ||
| delete(visited, ptr) | ||
|
|
||
| return nil |
There was a problem hiding this comment.
detectCyclesInEntity returns early for any non-pointer value (v.Kind() != reflect.Ptr), which means it will never traverse interface-typed relationships (where reflect.ValueOf(x).Kind() is Interface). Several entity structs contain relationships stored as interfaces (e.g., IPAddress.AssignedObject anyIPAddressAssignedObjectValue in diode/ingester.go:4180), so cycles reachable through those fields will not be detected and can still cause recursive proto conversion/stack overflows. Consider unwrapping Interface values (and traversing them) before the pointer check, and extend traversal to cover map values and slice/interface elements as well.
| // Get the pointer value of the entity using reflection | |
| v := reflect.ValueOf(entity) | |
| if v.Kind() != reflect.Ptr { | |
| return nil | |
| } | |
| if v.IsNil() { | |
| return nil | |
| } | |
| ptr := v.Pointer() | |
| // Check if we've seen this entity in the current path (cycle detected) | |
| if visited[ptr] { | |
| return fmt.Errorf("circular reference detected: %s", strings.Join(path, " -> ")) | |
| } | |
| // Mark as visited | |
| visited[ptr] = true | |
| // Get the entity type name for error messages | |
| elem := v.Elem() | |
| // Only walk through struct fields | |
| if elem.Kind() != reflect.Struct { | |
| // Still need to unmark before returning | |
| delete(visited, ptr) | |
| return nil | |
| } | |
| typeName := elem.Type().Name() | |
| newPath := append(path, typeName) | |
| // Walk through all fields of the entity struct | |
| for i := 0; i < elem.NumField(); i++ { | |
| field := elem.Field(i) | |
| // Handle pointer fields | |
| if field.Kind() == reflect.Ptr && !field.IsNil() { | |
| // Check if this field implements Entity interface | |
| if field.CanInterface() { | |
| fieldValue := field.Interface() | |
| if _, ok := fieldValue.(Entity); ok { | |
| if err := detectCyclesInEntity(fieldValue, visited, newPath); err != nil { | |
| return err | |
| } | |
| } else { | |
| // For non-Entity types, still recurse to check nested entities | |
| if err := detectCyclesInEntity(fieldValue, visited, newPath); err != nil { | |
| return err | |
| } | |
| } | |
| } | |
| } | |
| // Handle slice fields (e.g., []*Tag, []*VLAN) | |
| if field.Kind() == reflect.Slice { | |
| for j := 0; j < field.Len(); j++ { | |
| item := field.Index(j) | |
| if item.Kind() == reflect.Ptr && !item.IsNil() && item.CanInterface() { | |
| if err := detectCyclesInEntity(item.Interface(), visited, newPath); err != nil { | |
| return err | |
| } | |
| } | |
| } | |
| } | |
| } | |
| // Unmark after visiting all children (allow same entity in different branches) | |
| delete(visited, ptr) | |
| return nil | |
| v := reflect.ValueOf(entity) | |
| // Unwrap interface values to their concrete underlying value | |
| for v.Kind() == reflect.Interface { | |
| if v.IsNil() { | |
| return nil | |
| } | |
| v = v.Elem() | |
| } | |
| return walkValueForCycles(v, visited, path) | |
| } | |
| // walkValueForCycles walks an arbitrary reflect.Value to detect pointer cycles. | |
| func walkValueForCycles(v reflect.Value, visited map[uintptr]bool, path []string) error { | |
| switch v.Kind() { | |
| case reflect.Ptr: | |
| if v.IsNil() { | |
| return nil | |
| } | |
| ptr := v.Pointer() | |
| // Check if we've seen this entity in the current path (cycle detected) | |
| if visited[ptr] { | |
| return fmt.Errorf("circular reference detected: %s", strings.Join(path, " -> ")) | |
| } | |
| // Mark as visited | |
| visited[ptr] = true | |
| elem := v.Elem() | |
| // Extend the path with the underlying type name if available | |
| typeName := elem.Type().Name() | |
| newPath := path | |
| if typeName != "" { | |
| newPath = append(path, typeName) | |
| } | |
| if err := walkValueForCycles(elem, visited, newPath); err != nil { | |
| // Unmark before returning on error | |
| delete(visited, ptr) | |
| return err | |
| } | |
| // Unmark after visiting all children (allow same entity in different branches) | |
| delete(visited, ptr) | |
| return nil | |
| case reflect.Struct: | |
| // For non-pointer structs we still walk their fields, but they | |
| // themselves cannot participate directly in cycles without pointers. | |
| typeName := v.Type().Name() | |
| newPath := path | |
| if typeName != "" { | |
| newPath = append(path, typeName) | |
| } | |
| for i := 0; i < v.NumField(); i++ { | |
| field := v.Field(i) | |
| if !field.CanInterface() { | |
| continue | |
| } | |
| if err := walkValueForCycles(field, visited, newPath); err != nil { | |
| return err | |
| } | |
| } | |
| return nil | |
| case reflect.Slice, reflect.Array: | |
| for i := 0; i < v.Len(); i++ { | |
| elem := v.Index(i) | |
| if err := walkValueForCycles(elem, visited, path); err != nil { | |
| return err | |
| } | |
| } | |
| return nil | |
| case reflect.Map: | |
| // Traverse both keys and values, as either may hold pointers/entities. | |
| for _, key := range v.MapKeys() { | |
| if err := walkValueForCycles(key, visited, path); err != nil { | |
| return err | |
| } | |
| val := v.MapIndex(key) | |
| if val.IsValid() { | |
| if err := walkValueForCycles(val, visited, path); err != nil { | |
| return err | |
| } | |
| } | |
| } | |
| return nil | |
| case reflect.Interface: | |
| if v.IsNil() { | |
| return nil | |
| } | |
| return walkValueForCycles(v.Elem(), visited, path) | |
| default: | |
| // Primitive types and unsupported kinds cannot introduce new cycles. | |
| return nil | |
| } |
| // Check if this field implements Entity interface | ||
| if field.CanInterface() { | ||
| fieldValue := field.Interface() | ||
| if _, ok := fieldValue.(Entity); ok { | ||
| if err := detectCyclesInEntity(fieldValue, visited, newPath); err != nil { | ||
| return err | ||
| } | ||
| } else { | ||
| // For non-Entity types, still recurse to check nested entities | ||
| if err := detectCyclesInEntity(fieldValue, visited, newPath); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
The if _, ok := fieldValue.(Entity); ok { ... } else { ... } branch is redundant because both sides call detectCyclesInEntity with the same arguments. This adds noise and makes the traversal logic harder to reason about; simplify to a single call (and, if needed, add type-based filtering separately).
| // Check if this field implements Entity interface | |
| if field.CanInterface() { | |
| fieldValue := field.Interface() | |
| if _, ok := fieldValue.(Entity); ok { | |
| if err := detectCyclesInEntity(fieldValue, visited, newPath); err != nil { | |
| return err | |
| } | |
| } else { | |
| // For non-Entity types, still recurse to check nested entities | |
| if err := detectCyclesInEntity(fieldValue, visited, newPath); err != nil { | |
| return err | |
| } | |
| if field.CanInterface() { | |
| fieldValue := field.Interface() | |
| if err := detectCyclesInEntity(fieldValue, visited, newPath); err != nil { | |
| return err |
| func TestCircularReferenceDetection(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| entities []Entity | ||
| wantErr bool | ||
| errMsg string | ||
| }{ |
There was a problem hiding this comment.
TestCircularReferenceDetection covers pointer-to-pointer cycles, but it doesn’t cover cycles introduced via interface-typed relationship fields (e.g., AssignedObject, Termination, ParentObject) or via CustomFields (map -> CustomFieldValue -> CustomFieldObjectReference.Object interface). Since those shapes exist in the entity structs and are traversed during proto conversion, add at least one test case that creates a cycle through one of these interface/map paths to prevent regressions.
1 participant
This pull request introduces robust cycle detection for entity graphs during proto conversion, preventing issues from circular references when ingesting entities. The main changes include updating the
convertEntitiesToProtofunction to return errors on cycles, adding recursive cycle detection logic, updating client code to handle errors, and adding comprehensive tests for circular reference scenarios.Performance Cons
O(N)overhead on every Ingest call - The cycle detection traverses the entire entity graph using reflection before every ingestion, even for simple cases with no cyclesreflectpackage is slower than direct field access, adding latency to every ingest operationEntity conversion and cycle detection
convertEntitiesToPrototo return an error if a circular reference is detected in the entity graph, using a new recursivedetectCycleshelper. This improves reliability by preventing infinite loops and stack overflows during proto conversion.reflectimport to support runtime introspection for cycle detection logic.Client error handling
Ingestmethods inGRPCClient,DryRunClient, andOTLPClientto handle conversion errors, returning a descriptive error if cycle detection fails. [1] [2] [3]Testing improvements
TestConvertEntitiesToPrototo check for errors returned by the new conversion logic.TestCircularReferenceDetectionto verify cycle detection works for various scenarios, including cycles and shared references.