carverauto/serviceradar

Fork 0

Bug/core restarts demo #2510

Merged

mfreeman451 merged 2 commits from refs/pull/2510/head into main

2025-12-05 00:54:18 +00:00

mfreeman451 commented

2025-12-05 00:46:36 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2063
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2063
Original created: 2025-12-05T00:46:36Z
Original updated: 2025-12-05T00:54:23Z
Original head: carverauto/serviceradar:bug/core_restarts_demo
Original base: main
Original merged: 2025-12-05T00:54:18Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

I have signed the DCO?
The build completes without errors?
All tests are passing when running make test?

PR Type

Bug fix, Enhancement

Description

Prevent OOM crashes by implementing memory-aware backpressure in AGE graph writer
- Add memory limit threshold to reject batches when heap exceeds configured limit
- Implement circuit breaker pattern to disable writes after consecutive failures
Increase default workers from 1 to 4 and reduce queue size from 512 to 256
Add comprehensive metrics for dropped batches, heap memory, and circuit state
Document requirements and implementation tasks for OOM fix

Diagram Walkthrough

flowchart LR
  A["AGE Graph Writer"] --> B{"Check Circuit<br/>Breaker"}
  B -->|Open| C["Drop Batch<br/>Record Metric"]
  B -->|Closed| D{"Check Memory<br/>Pressure"}
  D -->|Exceeded| C
  D -->|OK| E["Enqueue to<br/>Work Queue"]
  E --> F["4 Worker<br/>Goroutines"]
  F --> G["Process Batch"]
  G --> H{"Success?"}
  H -->|Yes| I["Reset Circuit"]
  H -->|No| J["Increment<br/>Failures"]
  J --> K{"Threshold<br/>Reached?"}
  K -->|Yes| L["Open Circuit"]

File Walkthrough

Relevant files

Enhancement

age_graph_metrics.go

Add memory and circuit breaker metrics to AGE graph writer

pkg/registry/age_graph_metrics.go

Add runtime memory stats import to track heap allocation
Define 6 new metric constants for dropped batches, memory pressure,
and circuit state
Add atomic counters for tracking dropped batches by reason
Implement 6 new observable gauges for backpressure, memory, and
circuit metrics
Add helper functions to record dropped batches and manage circuit
state
Integrate heap allocation and circuit state into metric callback

+130/-12

Bug fix

age_graph_writer.go

Implement memory backpressure and circuit breaker in AGE writer

pkg/registry/age_graph_writer.go

Increase default workers from 1 to 4 and queue size from 512 to 256
Add memory limit, circuit threshold, and reset time configuration
parameters
Implement memory pressure check in enqueue() to reject batches when
heap exceeds limit
Add circuit breaker state management with closed/open/half-open states
Implement circuit breaker logic that opens after N consecutive
failures and half-opens after timeout
Record dropped batches for backpressure, memory pressure, and circuit
open conditions
Reset circuit on successful batch processing

+152/-17

Documentation

proposal.md `Document AGE graph OOM root cause and mitigation strategy` openspec/changes/fix-core-oom-age-graph-backpressure/proposal.md Document root cause of OOM crashes: single worker bottleneck with queue backlog Explain memory accumulation from goroutines holding payload data during long timeouts Describe retry amplification extending memory retention Outline solution: increase workers, add memory-aware backpressure, implement circuit breaker List affected specs and code files	+26/-0
spec.md `Add AGE graph memory backpressure and circuit breaker requirements` openspec/changes/fix-core-oom-age-graph-backpressure/specs/device-relationship-graph/spec.md Add requirement for memory-bounded queueing with early rejection at threshold Define scenarios for high ingestion rate and memory pressure handling Add requirement for parallel worker scaling to improve queue drain rate Add requirement for payload chunking to limit per-request memory footprint Add requirement for circuit breaker pattern with open/half-open/closed states Define scenarios for circuit opening after failures and closing after recovery	+46/-0
tasks.md `Track completion of AGE graph OOM fix implementation tasks` openspec/changes/fix-core-oom-age-graph-backpressure/tasks.md Mark completed tasks for worker scaling, queue size reduction, and memory metrics Mark completed tasks for memory limit configuration and backpressure implementation Mark completed tasks for circuit breaker implementation with state transitions Leave pending tasks for streaming payload processing and metric validation	+24/-0

Imported from GitHub pull request. Original GitHub pull request: #2063 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2063 Original created: 2025-12-05T00:46:36Z Original updated: 2025-12-05T00:54:23Z Original head: carverauto/serviceradar:bug/core_restarts_demo Original base: main Original merged: 2025-12-05T00:54:18Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Bug fix, Enhancement ___ ### **Description** - Prevent OOM crashes by implementing memory-aware backpressure in AGE graph writer - Add memory limit threshold to reject batches when heap exceeds configured limit - Implement circuit breaker pattern to disable writes after consecutive failures - Increase default workers from 1 to 4 and reduce queue size from 512 to 256 - Add comprehensive metrics for dropped batches, heap memory, and circuit state - Document requirements and implementation tasks for OOM fix ___ ### Diagram Walkthrough ```mermaid flowchart LR A["AGE Graph Writer"] --> B{"Check Circuit Breaker"} B -->|Open| C["Drop Batch Record Metric"] B -->|Closed| D{"Check Memory Pressure"} D -->|Exceeded| C D -->|OK| E["Enqueue to Work Queue"] E --> F["4 Worker Goroutines"] F --> G["Process Batch"] G --> H{"Success?"} H -->|Yes| I["Reset Circuit"] H -->|No| J["Increment Failures"] J --> K{"Threshold Reached?"} K -->|Yes| L["Open Circuit"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td>Enhancement</td><td><table> <tr> <td> <details> <summary>age_graph_metrics.go<dd><code>Add memory and circuit breaker metrics to AGE graph writer</code></dd></summary> <hr> pkg/registry/age_graph_metrics.go <ul><li>Add runtime memory stats import to track heap allocation <li> Define 6 new metric constants for dropped batches, memory pressure, and circuit state <li> Add atomic counters for tracking dropped batches by reason <li> Implement 6 new observable gauges for backpressure, memory, and circuit metrics <li> Add helper functions to record dropped batches and manage circuit state <li> Integrate heap allocation and circuit state into metric callback</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2063/files#diff-2d400ecd96ff77c1379e4a8681763f9ba2dc22790e31a445b7e3ce716e8fbb15">+130/-12</a></td> </tr> </table></td></tr><tr><td>Bug fix</td><td><table> <tr> <td> <details> <summary>age_graph_writer.go<dd><code>Implement memory backpressure and circuit breaker in AGE writer</code></dd></summary> <hr> pkg/registry/age_graph_writer.go <ul><li>Increase default workers from 1 to 4 and queue size from 512 to 256 <li> Add memory limit, circuit threshold, and reset time configuration parameters <li> Implement memory pressure check in enqueue() to reject batches when heap exceeds limit <li> Add circuit breaker state management with closed/open/half-open states <li> Implement circuit breaker logic that opens after N consecutive failures and half-opens after timeout <li> Record dropped batches for backpressure, memory pressure, and circuit open conditions <li> Reset circuit on successful batch processing</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2063/files#diff-2e6c0a5c048582201ffb983889ab9fab6fb1f8d64b91ee3fb65479b2fb2c6195">+152/-17</a></td> </tr> </table></td></tr><tr><td>Documentation</td><td><table> <tr> <td> <details> <summary>proposal.md<dd><code>Document AGE graph OOM root cause and mitigation strategy</code></dd></summary> <hr> openspec/changes/fix-core-oom-age-graph-backpressure/proposal.md <ul><li>Document root cause of OOM crashes: single worker bottleneck with queue backlog <li> Explain memory accumulation from goroutines holding payload data during long timeouts <li> Describe retry amplification extending memory retention <li> Outline solution: increase workers, add memory-aware backpressure, implement circuit breaker <li> List affected specs and code files</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2063/files#diff-81ac6baf30e8d593daac5904fe8ee44954ea59b60cfebb932d6167039401ec70">+26/-0</a>    </td> </tr> <tr> <td> <details> <summary>spec.md<dd><code>Add AGE graph memory backpressure and circuit breaker requirements</code></dd></summary> <hr> openspec/changes/fix-core-oom-age-graph-backpressure/specs/device-relationship-graph/spec.md <ul><li>Add requirement for memory-bounded queueing with early rejection at threshold <li> Define scenarios for high ingestion rate and memory pressure handling <li> Add requirement for parallel worker scaling to improve queue drain rate <li> Add requirement for payload chunking to limit per-request memory footprint <li> Add requirement for circuit breaker pattern with open/half-open/closed states <li> Define scenarios for circuit opening after failures and closing after recovery</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2063/files#diff-b476498724d1ccfae69ca3db18d65fc2a18e629803fe277531948545ca36b944">+46/-0</a>    </td> </tr> <tr> <td> <details> <summary>tasks.md<dd><code>Track completion of AGE graph OOM fix implementation tasks</code></dd></summary> <hr> openspec/changes/fix-core-oom-age-graph-backpressure/tasks.md <ul><li>Mark completed tasks for worker scaling, queue size reduction, and memory metrics <li> Mark completed tasks for memory limit configuration and backpressure implementation <li> Mark completed tasks for circuit breaker implementation with state transitions <li> Leave pending tasks for streaming payload processing and metric validation</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2063/files#diff-e7a2e1f30c6f53270296c7e92cf5828c71a4a21dc4ea2a6c1b4d74da304e87ac">+24/-0</a>    </td> </tr> </table></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-12-05 00:47:11 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2063#issuecomment-3614879303
Original created: 2025-12-05T00:47:11Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Rejection handling: New early-rejection paths for memory pressure and circuit-open return errors without visible upstream handling in this diff, which may drop critical work without retry or alternative path. Referred Code func (w *ageGraphWriter) enqueue(ctx context.Context, kind string, size int, query, payload string) error { if w == nil \|\| w.executor == nil { return errAgeGraphWriterUnavailable } // Check circuit breaker state if w.isCircuitOpen() { recordAgeDroppedCircuitOpen() return errAgeGraphCircuitOpen } // Check memory pressure if w.memoryLimitBytes > 0 { heapBytes := currentHeapAllocBytes() if heapBytes > w.memoryLimitBytes { recordAgeDroppedMemoryPressure() if w.log != nil { w.log.Warn(). Uint64("heap_bytes", heapBytes). Uint64("limit_bytes", w.memoryLimitBytes). Str("kind", kind). ... (clipped 6 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Runtime metrics: New memory-based gating uses runtime heap stats without validating external inputs for 'kind' or 'payload' in the enqueue path visible here; validation may exist elsewhere but is not shown. Referred Code `if w.memoryLimitBytes > 0 { heapBytes := currentHeapAllocBytes() if heapBytes > w.memoryLimitBytes { recordAgeDroppedMemoryPressure() if w.log != nil { w.log.Warn(). Uint64("heap_bytes", heapBytes). Uint64("limit_bytes", w.memoryLimitBytes). Str("kind", kind). Int("batch_size", size). Msg("age graph: rejected batch due to memory pressure") } return errAgeGraphMemoryPressure` Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2063#issuecomment-3614879303 Original created: 2025-12-05T00:47:11Z --- ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR: <table><tbody><tr><td colspan='2'>Security Compliance</td></tr> <tr><td>🟢</td><td><details><summary>No security concerns identified</summary> No security vulnerabilities detected by AI analysis. Human verification advised for critical code. </details></td></tr> <tr><td colspan='2'>Ticket Compliance</td></tr> <tr><td>⚪</td><td><details><summary>🎫 No ticket provided </summary> - [ ] Create ticket/issue  </details></td></tr> <tr><td colspan='2'>Codebase Duplication Compliance</td></tr> <tr><td>⚪</td><td><details><summary>Codebase context is not defined </summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'>Custom Compliance</td></tr> <tr><td rowspan=4>🟢</td><td> <details><summary>Generic: Comprehensive Audit Trails</summary> **Objective:** To create a detailed and reliable record of critical system actions for security analysis and compliance. **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary>Generic: Meaningful Naming and Self-Documenting Code</summary> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code self-documenting **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary>Generic: Secure Error Handling</summary> **Objective:** To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary>Generic: Secure Logging Practices</summary> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=2>⚪</td> <td><details> <summary>Generic: Robust Error Handling and Edge Case Management</summary> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful degradation **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2063/files#diff-2e6c0a5c048582201ffb983889ab9fab6fb1f8d64b91ee3fb65479b2fb2c6195R789-R815'>Rejection handling</a>: New early-rejection paths for memory pressure and circuit-open return errors without visible upstream handling in this diff, which may drop critical work without retry or alternative path. <details open><summary>Referred Code</summary> ```go func (w *ageGraphWriter) enqueue(ctx context.Context, kind string, size int, query, payload string) error { if w == nil || w.executor == nil { return errAgeGraphWriterUnavailable } // Check circuit breaker state if w.isCircuitOpen() { recordAgeDroppedCircuitOpen() return errAgeGraphCircuitOpen } // Check memory pressure if w.memoryLimitBytes > 0 { heapBytes := currentHeapAllocBytes() if heapBytes > w.memoryLimitBytes { recordAgeDroppedMemoryPressure() if w.log != nil { w.log.Warn(). Uint64("heap_bytes", heapBytes). Uint64("limit_bytes", w.memoryLimitBytes). Str("kind", kind). ... (clipped 6 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary>Generic: Security-First Input Validation and Data Handling</summary> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2063/files#diff-2e6c0a5c048582201ffb983889ab9fab6fb1f8d64b91ee3fb65479b2fb2c6195R801-R813'>Runtime metrics</a>: New memory-based gating uses runtime heap stats without validating external inputs for 'kind' or 'payload' in the enqueue path visible here; validation may exist elsewhere but is not shown. <details open><summary>Referred Code</summary> ```go if w.memoryLimitBytes > 0 { heapBytes := currentHeapAllocBytes() if heapBytes > w.memoryLimitBytes { recordAgeDroppedMemoryPressure() if w.log != nil { w.log.Warn(). Uint64("heap_bytes", heapBytes). Uint64("limit_bytes", w.memoryLimitBytes). Str("kind", kind). Int("batch_size", size). Msg("age graph: rejected batch due to memory pressure") } return errAgeGraphMemoryPressure ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td align="center" colspan="2">   </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant 🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label </details>

qodo-code-review[bot] commented

2025-12-05 00:48:25 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/2063#issuecomment-3614881618
Original created: 2025-12-05T00:48:25Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix circuit breaker race condition Fix a race condition in the circuit breaker's half-open state transition by using an atomic compare-and-swap operation, ensuring only a single request can pass through to test for recovery. pkg/registry/age_graph_writer.go [1017-1047] func (w ageGraphWriter) isCircuitOpen() bool { if w == nil \|\| w.circuitThreshold <= 0 { return false } state := currentAgeCircuitState() if state == circuitClosed { return false } if state == circuitOpen { // Check if reset time has passed openedAt := w.circuitOpenedAt.Load() if openedAt > 0 && time.Now().Unix()-openedAt >= int64(w.circuitResetSecs) { - // Transition to half-open - w.circuitMu.Lock() - if currentAgeCircuitState() == circuitOpen { - setAgeCircuitState(circuitHalfOpen) + // Atomically transition to half-open. Only one goroutine will succeed. + if ageCircuitState.CompareAndSwap(circuitOpen, circuitHalfOpen) { if w.log != nil { w.log.Info().Msg("age graph: circuit breaker transitioning to half-open") } + return false // Allow one request through } - w.circuitMu.Unlock() - return false // Allow one request through } return true } - // Half-open: allow requests through to test recovery - return false + // In half-open state, reject all other requests until state changes. + return true } Apply / Chat* Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies a race condition in the circuit breaker logic that allows multiple requests in the half-open state, defeating its purpose, and proposes a valid atomic fix.	High
General	Remove unnecessary lock in hot path Improve performance by replacing the mutex in the `resetCircuit` function with more efficient atomic operations, as it is on a hot path called by every successful request. pkg/registry/age_graph_writer.go [1070-1087] func (w ageGraphWriter) resetCircuit() { if w == nil \|\| w.circuitThreshold <= 0 { return } - w.circuitMu.Lock() - defer w.circuitMu.Unlock() - - prevState := currentAgeCircuitState() - if w.circuitFailures > 0 \|\| prevState != circuitClosed { - w.circuitFailures = 0 + // Use CAS to avoid locking on the hot path. + // Only log if the state was not already closed. + if ageCircuitState.CompareAndSwap(circuitHalfOpen, circuitClosed) { + atomic.StoreInt64(&w.circuitFailures, 0) w.circuitOpenedAt.Store(0) - setAgeCircuitState(circuitClosed) - if prevState != circuitClosed && w.log != nil { + if w.log != nil { w.log.Info().Msg("age graph: circuit breaker closed after successful request") + } + } else { + // Reset failure count if it was non-zero in closed state. + if atomic.LoadInt64(&w.circuitFailures) > 0 { + atomic.StoreInt64(&w.circuitFailures, 0) } } } Apply / Chat* Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that using a mutex on a hot path for every successful request is inefficient and proposes a more performant solution using atomic operations, which is a valid optimization.	Medium
General	Avoid expensive call on hot path Optimize the memory pressure check by reading memory stats in a background goroutine and caching the result, avoiding frequent and potentially expensive calls to `runtime.ReadMemStats()` on the hot path. pkg/registry/age_graph_writer.go [800-815] `// Check memory pressure if w.memoryLimitBytes > 0 { - heapBytes := currentHeapAllocBytes() + heapBytes := atomic.LoadUint64(&w.currentHeapAlloc) if heapBytes > w.memoryLimitBytes { recordAgeDroppedMemoryPressure() if w.log != nil { w.log.Warn(). Uint64("heap_bytes", heapBytes). Uint64("limit_bytes", w.memoryLimitBytes). Str("kind", kind). Int("batch_size", size). Msg("age graph: rejected batch due to memory pressure") } return errAgeGraphMemoryPressure } }` Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a potential performance bottleneck from calling `runtime.ReadMemStats()` on a hot path and proposes a valid optimization using a background goroutine to cache the value.	Low
High-level	Consider a simpler circuit breaker Replace the manual circuit breaker implementation, which uses mutexes and atomic operations for state management, with a standard third-party library. This would simplify the code and reduce the risk of concurrency issues. Examples: pkg/registry/age_graph_writer.go [1017-1087] `func (w ageGraphWriter) isCircuitOpen() bool { if w == nil \|\| w.circuitThreshold <= 0 { return false } state := currentAgeCircuitState() if state == circuitClosed { return false } ... (clipped 61 lines)` Solution Walkthrough: Before: type ageGraphWriter struct { // ... circuitFailures int64 circuitThreshold int circuitOpenedAt atomic.Int64 circuitMu sync.Mutex } func (w ageGraphWriter) isCircuitOpen() bool { // Manual check of state (closed, open, half-open) // Manual check of timer to transition to half-open // Manual locking to change state } func (w ageGraphWriter) incrementCircuitFailures() { // Manual locking to increment failures and open circuit } func (w ageGraphWriter) resetCircuit() { // Manual locking to reset failures and close circuit } After: import "github.com/sony/gobreaker" type ageGraphWriter struct { // ... cb gobreaker.CircuitBreaker } func NewAGEGraphWriter(...) GraphWriter { st := gobreaker.Settings{ Name: "AGEGraphWriter", MaxRequests: 1, // for half-open state Interval: 0, // never move to open state based on time Timeout: 60 time.Second, // from open to half-open ReadyToTrip: func(counts gobreaker.Counts) bool { return counts.ConsecutiveFailures > 10 }, } return &ageGraphWriter{..., cb: gobreaker.NewCircuitBreaker(st)} } func (w ageGraphWriter) processRequest(req ageGraphRequest) error { _, err := w.cb.Execute(func() (interface{}, error) { // ... original processing logic ... return nil, err }) return err } Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies a complex, manual implementation of a standard pattern and proposes a valid alternative that would improve code simplicity, robustness, and maintainability.	Low
More

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2063#issuecomment-3614881618 Original created: 2025-12-05T00:48:25Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td>Category</td><td align=left>Suggestion                                                                                                                                    </td><td align=center>Impact</td></tr><tbody><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Fix circuit breaker race condition</summary> ___ **Fix a race condition in the circuit breaker's half-open state transition by using an atomic compare-and-swap operation, ensuring only a single request can pass through to test for recovery.** [pkg/registry/age_graph_writer.go [1017-1047]](https://github.com/carverauto/serviceradar/pull/2063/files#diff-2e6c0a5c048582201ffb983889ab9fab6fb1f8d64b91ee3fb65479b2fb2c6195R1017-R1047) ```diff func (w *ageGraphWriter) isCircuitOpen() bool { if w == nil || w.circuitThreshold <= 0 { return false } state := currentAgeCircuitState() if state == circuitClosed { return false } if state == circuitOpen { // Check if reset time has passed openedAt := w.circuitOpenedAt.Load() if openedAt > 0 && time.Now().Unix()-openedAt >= int64(w.circuitResetSecs) { - // Transition to half-open - w.circuitMu.Lock() - if currentAgeCircuitState() == circuitOpen { - setAgeCircuitState(circuitHalfOpen) + // Atomically transition to half-open. Only one goroutine will succeed. + if ageCircuitState.CompareAndSwap(circuitOpen, circuitHalfOpen) { if w.log != nil { w.log.Info().Msg("age graph: circuit breaker transitioning to half-open") } + return false // Allow one request through } - w.circuitMu.Unlock() - return false // Allow one request through } return true } - // Half-open: allow requests through to test recovery - return false + // In half-open state, reject all other requests until state changes. + return true } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies a race condition in the circuit breaker logic that allows multiple requests in the half-open state, defeating its purpose, and proposes a valid atomic fix. </details></details></td><td align=center>High </td></tr><tr><td rowspan=2>General</td> <td> <details><summary>Remove unnecessary lock in hot path</summary> ___ **Improve performance by replacing the mutex in the <code>resetCircuit</code> function with more efficient atomic operations, as it is on a hot path called by every successful request.** [pkg/registry/age_graph_writer.go [1070-1087]](https://github.com/carverauto/serviceradar/pull/2063/files#diff-2e6c0a5c048582201ffb983889ab9fab6fb1f8d64b91ee3fb65479b2fb2c6195R1070-R1087) ```diff func (w *ageGraphWriter) resetCircuit() { if w == nil || w.circuitThreshold <= 0 { return } - w.circuitMu.Lock() - defer w.circuitMu.Unlock() - - prevState := currentAgeCircuitState() - if w.circuitFailures > 0 || prevState != circuitClosed { - w.circuitFailures = 0 + // Use CAS to avoid locking on the hot path. + // Only log if the state was not already closed. + if ageCircuitState.CompareAndSwap(circuitHalfOpen, circuitClosed) { + atomic.StoreInt64(&w.circuitFailures, 0) w.circuitOpenedAt.Store(0) - setAgeCircuitState(circuitClosed) - if prevState != circuitClosed && w.log != nil { + if w.log != nil { w.log.Info().Msg("age graph: circuit breaker closed after successful request") + } + } else { + // Reset failure count if it was non-zero in closed state. + if atomic.LoadInt64(&w.circuitFailures) > 0 { + atomic.StoreInt64(&w.circuitFailures, 0) } } } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly points out that using a mutex on a hot path for every successful request is inefficient and proposes a more performant solution using atomic operations, which is a valid optimization. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Avoid expensive call on hot path</summary> ___ **Optimize the memory pressure check by reading memory stats in a background goroutine and caching the result, avoiding frequent and potentially expensive calls to <code>runtime.ReadMemStats()</code> on the hot path.** [pkg/registry/age_graph_writer.go [800-815]](https://github.com/carverauto/serviceradar/pull/2063/files#diff-2e6c0a5c048582201ffb983889ab9fab6fb1f8d64b91ee3fb65479b2fb2c6195R800-R815) ```diff // Check memory pressure if w.memoryLimitBytes > 0 { - heapBytes := currentHeapAllocBytes() + heapBytes := atomic.LoadUint64(&w.currentHeapAlloc) if heapBytes > w.memoryLimitBytes { recordAgeDroppedMemoryPressure() if w.log != nil { w.log.Warn(). Uint64("heap_bytes", heapBytes). Uint64("limit_bytes", w.memoryLimitBytes). Str("kind", kind). Int("batch_size", size). Msg("age graph: rejected batch due to memory pressure") } return errAgeGraphMemoryPressure } } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies a potential performance bottleneck from calling `runtime.ReadMemStats()` on a hot path and proposes a valid optimization using a background goroutine to cache the value. </details></details></td><td align=center>Low </td></tr><tr><td rowspan=1>High-level</td> <td> <details><summary>Consider a simpler circuit breaker</summary> ___ **Replace the manual circuit breaker implementation, which uses mutexes and atomic operations for state management, with a standard third-party library. This would simplify the code and reduce the risk of concurrency issues.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2063/files#diff-2e6c0a5c048582201ffb983889ab9fab6fb1f8d64b91ee3fb65479b2fb2c6195R1017-R1087">pkg/registry/age_graph_writer.go [1017-1087]</a> </summary> ```go func (w *ageGraphWriter) isCircuitOpen() bool { if w == nil || w.circuitThreshold <= 0 { return false } state := currentAgeCircuitState() if state == circuitClosed { return false } ... (clipped 61 lines) ``` </details> ### Solution Walkthrough: #### Before: ```go type ageGraphWriter struct { // ... circuitFailures int64 circuitThreshold int circuitOpenedAt atomic.Int64 circuitMu sync.Mutex } func (w *ageGraphWriter) isCircuitOpen() bool { // Manual check of state (closed, open, half-open) // Manual check of timer to transition to half-open // Manual locking to change state } func (w *ageGraphWriter) incrementCircuitFailures() { // Manual locking to increment failures and open circuit } func (w *ageGraphWriter) resetCircuit() { // Manual locking to reset failures and close circuit } ``` #### After: ```go import "github.com/sony/gobreaker" type ageGraphWriter struct { // ... cb *gobreaker.CircuitBreaker } func NewAGEGraphWriter(...) GraphWriter { st := gobreaker.Settings{ Name: "AGEGraphWriter", MaxRequests: 1, // for half-open state Interval: 0, // never move to open state based on time Timeout: 60 * time.Second, // from open to half-open ReadyToTrip: func(counts gobreaker.Counts) bool { return counts.ConsecutiveFailures > 10 }, } return &ageGraphWriter{..., cb: gobreaker.NewCircuitBreaker(st)} } func (w *ageGraphWriter) processRequest(req *ageGraphRequest) error { _, err := w.cb.Execute(func() (interface{}, error) { // ... original processing logic ... return nil, err }) return err } ``` <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies a complex, manual implementation of a standard pattern and proposes a valid alternative that would improve code simplicity, robustness, and maintainability. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] More  </td><td></td></tr></tbody></table>