carverauto/serviceradar

Fork 0

initial #2582

Merged

mfreeman451 merged 1 commit from refs/pull/2582/head into staging

2025-12-16 19:51:21 +00:00

mfreeman451 commented

2025-12-16 19:48:30 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2158
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2158
Original created: 2025-12-16T19:48:30Z
Original updated: 2025-12-16T19:51:24Z
Original head: carverauto/serviceradar:2148-shallow-copies-of-hostresult-in-summary-collectionstream-cause-data-races
Original base: staging
Original merged: 2025-12-16T19:51:21Z 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

Description

Add DeepCopyHostResult() helper to prevent data races from shallow copies
Update summary collection paths to use deep copies before releasing locks
Update memory store conversions to use deep copies for consistency
Add regression tests verifying concurrent read safety and no aliasing

Diagram Walkthrough

flowchart LR
  A["HostResult with pointers<br/>PortResults, PortMap, ICMPStatus"]
  B["DeepCopyHostResult()<br/>helper function"]
  C["BaseProcessor<br/>collectShardSummaries"]
  D["BaseProcessor<br/>processShardForSummary"]
  E["InMemoryStore<br/>convertToSlice/buildSummary"]
  F["Safe snapshots<br/>no aliasing"]
  
  A -->|"deep copy"| B
  B -->|"used by"| C
  B -->|"used by"| D
  B -->|"used by"| E
  C -->|"produces"| F
  D -->|"produces"| F
  E -->|"produces"| F

File Walkthrough

Relevant files

Enhancement

sweep.go

Add deep-copy helper for HostResult

pkg/models/sweep.go

Add DeepCopyHostResult() function that creates non-aliased snapshots
of HostResult
Deep copies PortResults slice, PortMap map, and ICMPStatus struct
Ensures PortMap entries reference the same copied PortResult pointers
as PortResults
Minor formatting fix to ModeTCP constant alignment

+84/-1

Tests

sweep_deepcopy_test.go

Add unit tests for deep-copy function

pkg/models/sweep_deepcopy_test.go

Add comprehensive unit test TestDeepCopyHostResult_NoAliasing
Verify scalar fields match between source and copy
Verify pointer/slice/map fields are deep-copied, not aliased
Verify mutations to copy do not affect source
Test consistency between PortResults and PortMap references

+78/-0

summary_snapshot_test.go

Add regression and race detector tests

pkg/sweeper/summary_snapshot_test.go

Add TestGetSummary_HostResultsDoNotAliasInternalState regression test
Verify returned summaries have deep-copied ICMPStatus, PortMap, and
PortResults
Verify mutations to snapshot do not affect internal state
Add TestGetSummary_ConcurrentReadsDoNotPanic race detector test
Verify concurrent processing and summary reads do not cause panics or
races

+157/-0

Bug fix

base_processor.go `Use deep copies in summary collection paths` pkg/sweeper/base_processor.go Update `collectShardSummaries()` to use `DeepCopyHostResult()` when appending hosts Update `processShardForSummary()` to use `DeepCopyHostResult()` when streaming hosts Ensures deep copies are made before releasing shard locks Minor whitespace cleanup	+3/-3
memory_store.go `Use deep copies in memory store conversions` pkg/sweeper/memory_store.go Update `convertToSlice()` to use `DeepCopyHostResult()` for consistency Update `buildSummary()` to use `DeepCopyHostResult()` for consistency Fix struct field alignment in `InMemoryStore` definition Prevents accidental aliasing as code evolves	+15/-15

Documentation

proposal.md `Add change proposal documentation` openspec/changes/fix-sweeper-summary-shallow-copy/proposal.md Document issue #2148 data race root cause (shallow copies with pointer fields) Describe solution: introduce shared deep-copy helper and apply to summary paths List affected files and low-risk impact assessment Discuss trade-offs: increased allocation vs. lock-free, race-free summaries	+28/-0
spec.md `Add specification requirements for snapshot isolation` openspec/changes/fix-sweeper-summary-shallow-copy/specs/sweeper/spec.md Add requirement: Summary snapshot isolation Define scenario: `GetSummary` returns safe-to-read host snapshots Define scenario: Streamed `HostResult` values remain safe after locks released Define scenario: Caller mutations do not affect subsequent summaries	+21/-0
tasks.md `Add implementation task checklist` openspec/changes/fix-sweeper-summary-shallow-copy/tasks.md Document task checklist for deep-copy implementation Track completion of helper function, integration, and verification steps Reference specific test names and verification commands	+17/-0

Imported from GitHub pull request. Original GitHub pull request: #2158 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2158 Original created: 2025-12-16T19:48:30Z Original updated: 2025-12-16T19:51:24Z Original head: carverauto/serviceradar:2148-shallow-copies-of-hostresult-in-summary-collectionstream-cause-data-races Original base: staging Original merged: 2025-12-16T19:51:21Z 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 ___ ### **Description** - Add `DeepCopyHostResult()` helper to prevent data races from shallow copies - Update summary collection paths to use deep copies before releasing locks - Update memory store conversions to use deep copies for consistency - Add regression tests verifying concurrent read safety and no aliasing ___ ### Diagram Walkthrough ```mermaid flowchart LR A["HostResult with pointers<br/>PortResults, PortMap, ICMPStatus"] B["DeepCopyHostResult()<br/>helper function"] C["BaseProcessor<br/>collectShardSummaries"] D["BaseProcessor<br/>processShardForSummary"] E["InMemoryStore<br/>convertToSlice/buildSummary"] F["Safe snapshots<br/>no aliasing"] A -->|"deep copy"| B B -->|"used by"| C B -->|"used by"| D B -->|"used by"| E C -->|"produces"| F D -->|"produces"| F E -->|"produces"| F ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><table> <tr> <td> <details> <summary><strong>sweep.go</strong><dd><code>Add deep-copy helper for HostResult</code>                                            </dd></summary> <hr> pkg/models/sweep.go <ul><li>Add <code>DeepCopyHostResult()</code> function that creates non-aliased snapshots <br>of <code>HostResult</code><br> <li> Deep copies <code>PortResults</code> slice, <code>PortMap</code> map, and <code>ICMPStatus</code> struct<br> <li> Ensures <code>PortMap</code> entries reference the same copied <code>PortResult</code> pointers <br>as <code>PortResults</code><br> <li> Minor formatting fix to <code>ModeTCP</code> constant alignment</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2158/files#diff-3cc7dd0e748c9f77be9e384fed2703ab77375716524b70860153b6a1abae27ca">+84/-1</a>    </td> </tr> </table></td></tr><tr><td><strong>Tests</strong></td><td><table> <tr> <td> <details> <summary><strong>sweep_deepcopy_test.go</strong><dd><code>Add unit tests for deep-copy function</code>                                        </dd></summary> <hr> pkg/models/sweep_deepcopy_test.go <ul><li>Add comprehensive unit test <code>TestDeepCopyHostResult_NoAliasing</code><br> <li> Verify scalar fields match between source and copy<br> <li> Verify pointer/slice/map fields are deep-copied, not aliased<br> <li> Verify mutations to copy do not affect source<br> <li> Test consistency between <code>PortResults</code> and <code>PortMap</code> references</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2158/files#diff-45bfc89e5b79e84e249bab30c33c776bd8465af54f17693e0bbc97c4eed0f003">+78/-0</a>    </td> </tr> <tr> <td> <details> <summary><strong>summary_snapshot_test.go</strong><dd><code>Add regression and race detector tests</code>                                      </dd></summary> <hr> pkg/sweeper/summary_snapshot_test.go <ul><li>Add <code>TestGetSummary_HostResultsDoNotAliasInternalState</code> regression test<br> <li> Verify returned summaries have deep-copied <code>ICMPStatus</code>, <code>PortMap</code>, and <br><code>PortResults</code><br> <li> Verify mutations to snapshot do not affect internal state<br> <li> Add <code>TestGetSummary_ConcurrentReadsDoNotPanic</code> race detector test<br> <li> Verify concurrent processing and summary reads do not cause panics or <br>races</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2158/files#diff-eb3740146940283b92c377e3c550ebe123ef3e010feaa2974a3029ef9260b08d">+157/-0</a>  </td> </tr> </table></td></tr><tr><td><strong>Bug fix</strong></td><td><table> <tr> <td> <details> <summary><strong>base_processor.go</strong><dd><code>Use deep copies in summary collection paths</code>                            </dd></summary> <hr> pkg/sweeper/base_processor.go <ul><li>Update <code>collectShardSummaries()</code> to use <code>DeepCopyHostResult()</code> when <br>appending hosts<br> <li> Update <code>processShardForSummary()</code> to use <code>DeepCopyHostResult()</code> when <br>streaming hosts<br> <li> Ensures deep copies are made before releasing shard locks<br> <li> Minor whitespace cleanup</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2158/files#diff-40a89f977fa936cab19d62a659fb835665f646012674fbf3a8b09a8987917fbb">+3/-3</a>      </td> </tr> <tr> <td> <details> <summary><strong>memory_store.go</strong><dd><code>Use deep copies in memory store conversions</code>                            </dd></summary> <hr> pkg/sweeper/memory_store.go <ul><li>Update <code>convertToSlice()</code> to use <code>DeepCopyHostResult()</code> for consistency<br> <li> Update <code>buildSummary()</code> to use <code>DeepCopyHostResult()</code> for consistency<br> <li> Fix struct field alignment in <code>InMemoryStore</code> definition<br> <li> Prevents accidental aliasing as code evolves</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2158/files#diff-54c761e2155455ac0e86e2cf1405610fe315d966ab17d4f008c3dd7227a05389">+15/-15</a>  </td> </tr> </table></td></tr><tr><td><strong>Documentation</strong></td><td><table> <tr> <td> <details> <summary><strong>proposal.md</strong><dd><code>Add change proposal documentation</code>                                                </dd></summary> <hr> openspec/changes/fix-sweeper-summary-shallow-copy/proposal.md <ul><li>Document issue #2148 data race root cause (shallow copies with pointer <br>fields)<br> <li> Describe solution: introduce shared deep-copy helper and apply to <br>summary paths<br> <li> List affected files and low-risk impact assessment<br> <li> Discuss trade-offs: increased allocation vs. lock-free, race-free <br>summaries</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2158/files#diff-6303af01f1a835c5774bb184afb2e37b9707b7b7c4283e3ee537b2b221ba614b">+28/-0</a>    </td> </tr> <tr> <td> <details> <summary><strong>spec.md</strong><dd><code>Add specification requirements for snapshot isolation</code>        </dd></summary> <hr> openspec/changes/fix-sweeper-summary-shallow-copy/specs/sweeper/spec.md <ul><li>Add requirement: Summary snapshot isolation<br> <li> Define scenario: <code>GetSummary</code> returns safe-to-read host snapshots<br> <li> Define scenario: Streamed <code>HostResult</code> values remain safe after locks <br>released<br> <li> Define scenario: Caller mutations do not affect subsequent summaries</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2158/files#diff-34df7c0ca139956ca9981cd0accef64a55838bf3057b72fc9dfcfa77ff19bd82">+21/-0</a>    </td> </tr> <tr> <td> <details> <summary><strong>tasks.md</strong><dd><code>Add implementation task checklist</code>                                                </dd></summary> <hr> openspec/changes/fix-sweeper-summary-shallow-copy/tasks.md <ul><li>Document task checklist for deep-copy implementation<br> <li> Track completion of helper function, integration, and verification <br>steps<br> <li> Reference specific test names and verification commands</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2158/files#diff-116bbee895b6dbf4fa6aaa32bd8fa799e2d2181a66071fb5e3129a46cc9154d7">+17/-0</a>    </td> </tr> </table></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-12-16 19:49:06 +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/2158#issuecomment-3662125903
Original created: 2025-12-16T19:49:06Z

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

🟢

🎫 #2148

🟢	Fix data races caused by shallow copying `HostResult` in `collectShardSummaries` by ensuring returned summary host data does not alias internal mutable fields (`PortResults`, `PortMap`, `ICMPStatus`) after shard locks are released.
	Fix data races caused by shallow copying `HostResult` in `processShardForSummary` / summary streaming by ensuring streamed `HostResult` values do not alias internal mutable fields after shard locks are released.
	Ensure returned data is an independent snapshot so concurrent `Process()` updates (e.g., adding ports) cannot race with callers reading summaries/streams.
	Provide regression/unit tests demonstrating no aliasing and protecting against recurrence (including race-detector coverage consistent with the reported failure scenario).

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

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/2158#issuecomment-3662125903 Original created: 2025-12-16T19:49:06Z --- _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/2148>#2148</a></summary> <table width='100%'><tbody> <tr><td rowspan=4>🟢</td> <td>Fix data races caused by shallow copying <code>HostResult</code> in <code>collectShardSummaries</code> by ensuring <br>returned summary host data does not alias internal mutable fields (<code>PortResults</code>, <code>PortMap</code>, <br><code>ICMPStatus</code>) after shard locks are released.<br></td></tr> <tr><td>Fix data races caused by shallow copying <code>HostResult</code> in <code>processShardForSummary</code> / summary <br>streaming by ensuring streamed <code>HostResult</code> values do not alias internal mutable fields <br>after shard locks are released.<br></td></tr> <tr><td>Ensure returned data is an independent snapshot so concurrent <code>Process()</code> updates (e.g., <br>adding ports) cannot race with callers reading summaries/streams.<br></td></tr> <tr><td>Provide regression/unit tests demonstrating no aliasing and protecting against recurrence <br>(including race-detector coverage consistent with the reported failure scenario).<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">   </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 19:50:12 +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/2158#issuecomment-3662129376
Original created: 2025-12-16T19:50:12Z

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
General	Simplify and correct deep copy logic Refactor the `DeepCopyHostResult` function to simplify the logic for copying `PortResults` and `PortMap`. First, copy `PortResults` and track copied pointers. Then, copy `PortMap`, reusing already copied pointers or creating new ones for map-only entries without modifying the `PortResults` slice. pkg/models/sweep.go [179-233] - copiedPortResults := make(map[PortResult]PortResult) - copiedByPort := make(map[int]PortResult) - + // 1. Deep copy PortResults and create a map of original to copied pointers. if src.PortResults != nil { - dst.PortResults = make([]PortResult, 0, len(src.PortResults)) - for _, pr := range src.PortResults { + dst.PortResults = make([]PortResult, len(src.PortResults)) + copiedPortResults := make(map[PortResult]PortResult, len(src.PortResults)) + for i, pr := range src.PortResults { if pr == nil { - dst.PortResults = append(dst.PortResults, nil) continue } - - prCopy := &PortResult{ - Port: pr.Port, - Available: pr.Available, - RespTime: pr.RespTime, - Service: pr.Service, - } - - copiedPortResults[pr] = prCopy - copiedByPort[pr.Port] = prCopy - dst.PortResults = append(dst.PortResults, prCopy) + prCopy := pr // Create a shallow copy of the struct value + dst.PortResults[i] = &prCopy + copiedPortResults[pr] = &prCopy } } + // 2. Deep copy PortMap, reusing pointers from the copied PortResults where possible. if src.PortMap != nil { dst.PortMap = make(map[int]PortResult, len(src.PortMap)) for port, pr := range src.PortMap { if pr == nil { - dst.PortMap[port] = nil continue } + // If this PortResult was already copied (because it was in PortResults), + // reuse the pointer to maintain consistency. if prCopy, ok := copiedPortResults[pr]; ok { dst.PortMap[port] = prCopy continue } - if prCopy, ok := copiedByPort[port]; ok { - dst.PortMap[port] = prCopy - continue - } - - prCopy := &PortResult{ - Port: pr.Port, - Available: pr.Available, - RespTime: pr.RespTime, - Service: pr.Service, - } - - copiedPortResults[pr] = prCopy - copiedByPort[port] = prCopy - dst.PortMap[port] = prCopy - dst.PortResults = append(dst.PortResults, prCopy) + // Otherwise, this PortResult was in PortMap but not PortResults. + // Create a new copy for the map. + prCopy := pr + dst.PortMap[port] = &prCopy } } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a bug in the new deep copy logic and proposes a cleaner, more robust implementation that fixes the bug and simplifies the code, improving correctness and maintainability.	Medium
General	Move channel sends outside lock In `processShardForSummary`, refactor the logic to first collect all deep-copied hosts into a temporary slice while holding the read lock, then release the lock before sending the hosts to the channel. pkg/sweeper/base_processor.go [790-795] +// inside processShardForSummary(...) +shard := p.shards[shardIndex] +shard.mu.RLock() +// collect deep copies under lock +copies := make([]models.HostResult, 0, len(shard.hostMap)) for _, host := range shard.hostMap { + copies = append(copies, models.DeepCopyHostResult(host)) + if host.Available { + result.availableHosts++ + } +} +shard.mu.RUnlock() +// stream copies outside lock +for _, cp := range copies { select { - case hostCh <- models.DeepCopyHostResult(host): - if host.Available { - result.availableHosts++ - } + case hostCh <- cp: + case <-ctx.Done(): + return result, false + } +} `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential performance bottleneck by sending to a channel while holding a lock and proposes a good practice to release the lock sooner, improving concurrency.	Medium
More

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2158#issuecomment-3662129376 Original created: 2025-12-16T19:50:12Z --- _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=2>General</td> <td> <details><summary>Simplify and correct deep copy logic</summary> ___ **Refactor the <code>DeepCopyHostResult</code> function to simplify the logic for copying <br><code>PortResults</code> and <code>PortMap</code>. First, copy <code>PortResults</code> and track copied pointers. <br>Then, copy <code>PortMap</code>, reusing already copied pointers or creating new ones for <br>map-only entries without modifying the <code>PortResults</code> slice.** [pkg/models/sweep.go [179-233]](https://github.com/carverauto/serviceradar/pull/2158/files#diff-3cc7dd0e748c9f77be9e384fed2703ab77375716524b70860153b6a1abae27caR179-R233) ```diff - copiedPortResults := make(map[*PortResult]*PortResult) - copiedByPort := make(map[int]*PortResult) - + // 1. Deep copy PortResults and create a map of original to copied pointers. if src.PortResults != nil { - dst.PortResults = make([]*PortResult, 0, len(src.PortResults)) - for _, pr := range src.PortResults { + dst.PortResults = make([]*PortResult, len(src.PortResults)) + copiedPortResults := make(map[*PortResult]*PortResult, len(src.PortResults)) + for i, pr := range src.PortResults { if pr == nil { - dst.PortResults = append(dst.PortResults, nil) continue } - - prCopy := &PortResult{ - Port: pr.Port, - Available: pr.Available, - RespTime: pr.RespTime, - Service: pr.Service, - } - - copiedPortResults[pr] = prCopy - copiedByPort[pr.Port] = prCopy - dst.PortResults = append(dst.PortResults, prCopy) + prCopy := *pr // Create a shallow copy of the struct value + dst.PortResults[i] = &prCopy + copiedPortResults[pr] = &prCopy } } + // 2. Deep copy PortMap, reusing pointers from the copied PortResults where possible. if src.PortMap != nil { dst.PortMap = make(map[int]*PortResult, len(src.PortMap)) for port, pr := range src.PortMap { if pr == nil { - dst.PortMap[port] = nil continue } + // If this PortResult was already copied (because it was in PortResults), + // reuse the pointer to maintain consistency. if prCopy, ok := copiedPortResults[pr]; ok { dst.PortMap[port] = prCopy continue } - if prCopy, ok := copiedByPort[port]; ok { - dst.PortMap[port] = prCopy - continue - } - - prCopy := &PortResult{ - Port: pr.Port, - Available: pr.Available, - RespTime: pr.RespTime, - Service: pr.Service, - } - - copiedPortResults[pr] = prCopy - copiedByPort[port] = prCopy - dst.PortMap[port] = prCopy - dst.PortResults = append(dst.PortResults, prCopy) + // Otherwise, this PortResult was in PortMap but not PortResults. + // Create a new copy for the map. + prCopy := *pr + dst.PortMap[port] = &prCopy } } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies a bug in the new deep copy logic and proposes a cleaner, more robust implementation that fixes the bug and simplifies the code, improving correctness and maintainability. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Move channel sends outside lock</summary> ___ **In <code>processShardForSummary</code>, refactor the logic to first collect all deep-copied <br>hosts into a temporary slice while holding the read lock, then release the lock <br>before sending the hosts to the channel.** [pkg/sweeper/base_processor.go [790-795]](https://github.com/carverauto/serviceradar/pull/2158/files#diff-40a89f977fa936cab19d62a659fb835665f646012674fbf3a8b09a8987917fbbR790-R795) ```diff +// inside processShardForSummary(...) +shard := p.shards[shardIndex] +shard.mu.RLock() +// collect deep copies under lock +copies := make([]models.HostResult, 0, len(shard.hostMap)) for _, host := range shard.hostMap { + copies = append(copies, models.DeepCopyHostResult(host)) + if host.Available { + result.availableHosts++ + } +} +shard.mu.RUnlock() +// stream copies outside lock +for _, cp := range copies { select { - case hostCh <- models.DeepCopyHostResult(host): - if host.Available { - result.availableHosts++ - } + case hostCh <- cp: + case <-ctx.Done(): + return result, false + } +} ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies a potential performance bottleneck by sending to a channel while holding a lock and proposes a good practice to release the lock sooner, improving concurrency. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] More  </td><td></td></tr></tbody></table>