carverauto/serviceradar

Fork 0

initial #2580

Merged

mfreeman451 merged 2 commits from refs/pull/2580/head into staging

2025-12-16 06:25:59 +00:00

mfreeman451 commented

2025-12-16 06:16:16 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2156
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2156
Original created: 2025-12-16T06:16:16Z
Original updated: 2025-12-16T06:26:03Z
Original head: carverauto/serviceradar:2141-deadlock-in-snmpservicecheck-due-to-nested-rlock-when-a-writer-is-waiting
Original base: staging
Original merged: 2025-12-16T06:25:59Z 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, Tests

Description

Remove recursive RWMutex read locking in SNMPService.Check() to prevent deadlock
Check() no longer holds RLock while calling GetStatus(), avoiding write-preferring semantics deadlock
Add AST-based regression test to prevent recursive RWMutex read locking reintroduction
Document deadlock fix with proposal, requirements, and task tracking

Diagram Walkthrough

flowchart LR
  A["SNMPService.Check()"] -->|previously held RLock| B["GetStatus()"]
  B -->|nested RLock blocked| C["Waiting Writer"]
  C -->|write-preferring RWMutex| D["Deadlock"]
  A -->|now no RLock| E["GetStatus()"]
  E -->|independent locking| F["No Deadlock"]

File Walkthrough

Relevant files

Bug fix

service.go

Remove recursive RWMutex read locking from Check

pkg/checker/snmp/service.go

Removed s.mu.RLock() and defer s.mu.RUnlock() from Check() method
Added explanatory comment documenting why recursive RWMutex read
locking must be avoided
Check() now calls GetStatus() without holding a read lock

+3/-4

Tests

service_deadlock_test.go

Add AST-based regression test for deadlock prevention

pkg/checker/snmp/service_deadlock_test.go

New regression test using AST parsing to verify Check() does not hold
RLock while calling GetStatus()
Test parses service.go at runtime and inspects the Check() method's
AST
Fails if both s.mu.RLock() and GetStatus() call are detected in the
same method
Prevents accidental reintroduction of the deadlock bug

+104/-0

Documentation

proposal.md `Document SNMP checker deadlock fix proposal` openspec/changes/fix-snmp-check-deadlock/proposal.md Documents GitHub issue #2141 describing the deadlock scenario Explains root cause: recursive RWMutex read locking with write-preferring semantics Outlines the fix and regression test coverage requirements Assesses risk as low due to localized lock usage changes	+16/-0
spec.md `Add deadlock-free health check requirement specification` openspec/changes/fix-snmp-check-deadlock/specs/snmp-checker/spec.md Adds new requirement: SNMP checker health checks must be deadlock-free Defines scenario for health check execution during concurrent datapoint updates Specifies that `Check()` must return without blocking indefinitely	+10/-0
tasks.md `Document implementation tasks and validation checklist` openspec/changes/fix-snmp-check-deadlock/tasks.md Documents three task categories: Fix, Tests, and Validation Lists specific implementation tasks including removing recursive RLock and ensuring thread-safety Includes regression test requirements and validation steps All tasks marked as completed	+12/-0

Configuration changes

BUILD.bazel

Include deadlock regression test in build configuration

pkg/checker/snmp/BUILD.bazel

Added service_deadlock_test.go to the test sources list in the
snmp_test target
Ensures the new regression test is included in the build

+1/-0

Imported from GitHub pull request. Original GitHub pull request: #2156 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2156 Original created: 2025-12-16T06:16:16Z Original updated: 2025-12-16T06:26:03Z Original head: carverauto/serviceradar:2141-deadlock-in-snmpservicecheck-due-to-nested-rlock-when-a-writer-is-waiting Original base: staging Original merged: 2025-12-16T06:25:59Z 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, Tests ___ ### **Description** - Remove recursive RWMutex read locking in `SNMPService.Check()` to prevent deadlock - `Check()` no longer holds RLock while calling `GetStatus()`, avoiding write-preferring semantics deadlock - Add AST-based regression test to prevent recursive RWMutex read locking reintroduction - Document deadlock fix with proposal, requirements, and task tracking ___ ### Diagram Walkthrough ```mermaid flowchart LR A["SNMPService.Check()"] -->|previously held RLock| B["GetStatus()"] B -->|nested RLock blocked| C["Waiting Writer"] C -->|write-preferring RWMutex| D["Deadlock"] A -->|now no RLock| E["GetStatus()"] E -->|independent locking| F["No Deadlock"] ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix</strong></td><td><table> <tr> <td> <details> <summary><strong>service.go</strong><dd><code>Remove recursive RWMutex read locking from Check</code>                  </dd></summary> <hr> pkg/checker/snmp/service.go <ul><li>Removed <code>s.mu.RLock()</code> and <code>defer s.mu.RUnlock()</code> from <code>Check()</code> method<br> <li> Added explanatory comment documenting why recursive RWMutex read <br>locking must be avoided<br> <li> <code>Check()</code> now calls <code>GetStatus()</code> without holding a read lock</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2156/files#diff-c592649b582c62a85d1dd416ac7fb609e55531cb43c8199a15f4432ce8ae05d8">+3/-4</a>      </td> </tr> </table></td></tr><tr><td><strong>Tests</strong></td><td><table> <tr> <td> <details> <summary><strong>service_deadlock_test.go</strong><dd><code>Add AST-based regression test for deadlock prevention</code>        </dd></summary> <hr> pkg/checker/snmp/service_deadlock_test.go <ul><li>New regression test using AST parsing to verify <code>Check()</code> does not hold <br>RLock while calling <code>GetStatus()</code><br> <li> Test parses <code>service.go</code> at runtime and inspects the <code>Check()</code> method's <br>AST<br> <li> Fails if both <code>s.mu.RLock()</code> and <code>GetStatus()</code> call are detected in the <br>same method<br> <li> Prevents accidental reintroduction of the deadlock bug</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2156/files#diff-6a31b13924de8ba2b712fc5deb881056c50f7448bb61eb84efe82850eb08325e">+104/-0</a>  </td> </tr> </table></td></tr><tr><td><strong>Documentation</strong></td><td><table> <tr> <td> <details> <summary><strong>proposal.md</strong><dd><code>Document SNMP checker deadlock fix proposal</code>                            </dd></summary> <hr> openspec/changes/fix-snmp-check-deadlock/proposal.md <ul><li>Documents GitHub issue #2141 describing the deadlock scenario<br> <li> Explains root cause: recursive RWMutex read locking with <br>write-preferring semantics<br> <li> Outlines the fix and regression test coverage requirements<br> <li> Assesses risk as low due to localized lock usage changes</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2156/files#diff-0534d758c6b8e27938b14175fe5a6ff9bf0ee3a352da208c3f651cab9a460736">+16/-0</a>    </td> </tr> <tr> <td> <details> <summary><strong>spec.md</strong><dd><code>Add deadlock-free health check requirement specification</code>  </dd></summary> <hr> openspec/changes/fix-snmp-check-deadlock/specs/snmp-checker/spec.md <ul><li>Adds new requirement: SNMP checker health checks must be deadlock-free<br> <li> Defines scenario for health check execution during concurrent <br>datapoint updates<br> <li> Specifies that <code>Check()</code> must return without blocking indefinitely</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2156/files#diff-5845d29d43f16fd852471b164a176e879de47eb15be4d4bcc32e27130a6095ec">+10/-0</a>    </td> </tr> <tr> <td> <details> <summary><strong>tasks.md</strong><dd><code>Document implementation tasks and validation checklist</code>      </dd></summary> <hr> openspec/changes/fix-snmp-check-deadlock/tasks.md <ul><li>Documents three task categories: Fix, Tests, and Validation<br> <li> Lists specific implementation tasks including removing recursive RLock <br>and ensuring thread-safety<br> <li> Includes regression test requirements and validation steps<br> <li> All tasks marked as completed</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2156/files#diff-f7b9a49b7e16fb0ce430f87e5fe38160d9a072efe92259ca776fe6ee0449a7c9">+12/-0</a>    </td> </tr> </table></td></tr><tr><td><strong>Configuration changes</strong></td><td><table> <tr> <td> <details> <summary><strong>BUILD.bazel</strong><dd><code>Include deadlock regression test in build configuration</code>    </dd></summary> <hr> pkg/checker/snmp/BUILD.bazel <ul><li>Added <code>service_deadlock_test.go</code> to the test sources list in the <br><code>snmp_test</code> target<br> <li> Ensures the new regression test is included in the build</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2156/files#diff-bf78b9b2c6f1b0b501487be34f583e55b0735ad601abb7c7e9d27cb55a0ef57f">+1/-0</a>      </td> </tr> </table></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-12-16 06:16:51 +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/2156#issuecomment-3659007714
Original created: 2025-12-16T06:16:51Z

You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

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

🟡

🎫 #2141

🟢	Eliminate the deadlock in `SNMPService.Check()` caused by holding `s.mu.RLock()` while calling `GetStatus()` (which also `RLock()`s) under RWMutex write-preferring semantics.
🟢	Add/maintain regression coverage to prevent reintroducing the nested `RLock()` pattern that triggers the deadlock.
⚪	Ensure `Check()` completes (does not block indefinitely) when concurrent datapoint updates attempt to acquire the write lock (e.g., `handleDataPoint()`).

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: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

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: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Update

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/2156#issuecomment-3659007714 Original created: 2025-12-16T06:16:51Z --- _You are nearing your monthly Qodo Merge usage quota. For more information, please visit [here](https://qodo-merge-docs.qodo.ai/installation/qodo_merge/#cloud-users)._ ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR:<br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td>🟢</td><td><details><summary><strong>No security concerns identified</strong></summary> No security vulnerabilities detected by AI analysis. Human verification advised for critical code. </details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>🟡</td> <td> <details> <summary>🎫 <a href=https://github.com/carverauto/serviceradar/issues/2141>#2141</a></summary> <table width='100%'><tbody> <tr><td rowspan=2>🟢</td> <td>Eliminate the deadlock in <code>SNMPService.Check()</code> caused by holding <code>s.mu.RLock()</code> while calling <br><code>GetStatus()</code> (which also <code>RLock()</code>s) under RWMutex write-preferring semantics.<br></td></tr> <tr><td>Add/maintain regression coverage to prevent reintroducing the nested <code>RLock()</code> pattern that <br>triggers the deadlock.<br></td></tr> <tr><td rowspan=1>⚪</td> <td>Ensure <code>Check()</code> completes (does not block indefinitely) when concurrent datapoint updates <br>attempt to acquire the write lock (e.g., <code>handleDataPoint()</code>).<br></td></tr> </tbody></table> </details> </td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></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'><strong>Custom Compliance</strong></td></tr> <tr><td rowspan=6>🟢</td><td> <details><summary><strong>Generic: Comprehensive Audit Trails</strong></summary><br> **Objective:** To create a detailed and reliable record of critical system actions for security analysis <br>and compliance.<br> **Status:** Passed<br> > 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><strong>Generic: Meaningful Naming and Self-Documenting Code</strong></summary><br> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code <br>self-documenting<br> **Status:** Passed<br> > 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><strong>Generic: Robust Error Handling and Edge Case Management</strong></summary><br> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful <br>degradation<br> **Status:** Passed<br> > 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><strong>Generic: Secure Error Handling</strong></summary><br> **Objective:** To prevent the leakage of sensitive system information through error messages while <br>providing sufficient detail for internal debugging.<br> **Status:** Passed<br> > 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><strong>Generic: Secure Logging Practices</strong></summary><br> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive <br>information like PII, PHI, or cardholder data.<br> **Status:** Passed<br> > 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><strong>Generic: Security-First Input Validation and Data Handling</strong></summary><br> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent <br>vulnerabilities<br> **Status:** Passed<br> > 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"> - [ ] Update  </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant<br> 🟡 - Partial Compliant<br> 🔴 - Not Compliant<br> ⚪ - Requires Further Human Verification<br> 🏷️ - Compliance label<br> </details>

qodo-code-review[bot] commented

2025-12-16 06:17:38 +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/2156#issuecomment-3659009456
Original created: 2025-12-16T06:17:38Z

You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Embed source file to improve test robustness To improve test robustness, replace the use of `runtime.Caller` and file paths with `//go:embed` to embed the `service.go` source file directly into the test binary. pkg/checker/snmp/service_deadlock_test.go [33-41] -_, thisFile, _, ok := runtime.Caller(0) -if !ok { - t.Fatal("unable to locate test file path via runtime.Caller") -} -filePath := filepath.Join(filepath.Dir(thisFile), "service.go") -parsed, err := parser.ParseFile(fileSet, filePath, nil, 0) +//go:embed service.go +var serviceGoSource []byte + +// ... + +// in TestCheckDoesNotRLockAndCallGetStatus: +parsed, err := parser.ParseFile(fileSet, "service.go", serviceGoSource, 0) if err != nil { - t.Fatalf("parse %s: %v", filePath, err) + t.Fatalf("parse service.go: %v", err) } `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that using `runtime.Caller` to locate source files is fragile, especially in sandboxed build environments like Bazel, which this project uses. Using `//go:embed` makes the test more robust and self-contained.	Medium
Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2156#issuecomment-3659009456 Original created: 2025-12-16T06:17:38Z --- _You are nearing your monthly Qodo Merge usage quota. For more information, please visit [here](https://qodo-merge-docs.qodo.ai/installation/qodo_merge/#cloud-users)._ ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Embed source file to improve test robustness</summary> ___ **To improve test robustness, replace the use of <code>runtime.Caller</code> and file paths <br>with <code>//go:embed</code> to embed the <code>service.go</code> source file directly into the test <br>binary.** [pkg/checker/snmp/service_deadlock_test.go [33-41]](https://github.com/carverauto/serviceradar/pull/2156/files#diff-6a31b13924de8ba2b712fc5deb881056c50f7448bb61eb84efe82850eb08325eR33-R41) ```diff -_, thisFile, _, ok := runtime.Caller(0) -if !ok { - t.Fatal("unable to locate test file path via runtime.Caller") -} -filePath := filepath.Join(filepath.Dir(thisFile), "service.go") -parsed, err := parser.ParseFile(fileSet, filePath, nil, 0) +//go:embed service.go +var serviceGoSource []byte + +// ... + +// in TestCheckDoesNotRLockAndCallGetStatus: +parsed, err := parser.ParseFile(fileSet, "service.go", serviceGoSource, 0) if err != nil { - t.Fatalf("parse %s: %v", filePath, err) + t.Fatalf("parse service.go: %v", err) } ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies that using `runtime.Caller` to locate source files is fragile, especially in sandboxed build environments like Bazel, which this project uses. Using `//go:embed` makes the test more robust and self-contained. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>