Sysmonvm pkg refresh #2307

Merged

mfreeman451 merged 6 commits from refs/pull/2307/head into main

2025-10-12 17:59:56 +00:00

mfreeman451 commented

2025-10-12 06:44:32 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #1746
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1746
Original created: 2025-10-12T06:44:32Z
Original updated: 2025-10-12T17:59:59Z
Original head: carverauto/serviceradar:sysmonvm-pkg-refresh
Original base: main
Original merged: 2025-10-12T17:59:56Z by @mfreeman451

PR Type

Enhancement

Description

Embed macOS IOReport frequency sampler into Go binary
Remove standalone hostfreq binary and launchd service
Refactor cpufreq collector with cgo bridge
Update build system and packaging scripts

Diagram Walkthrough

flowchart LR
  A["Standalone hostfreq binary"] --> B["Embedded cgo sampler"]
  C["External launchd service"] --> D["Single checker process"]
  E["Command execution"] --> F["Direct C++ bridge"]
  B --> G["Simplified deployment"]
  D --> G
  F --> G

File Walkthrough

Relevant files

Enhancement

6 files

collector.go `Refactor collector to use embedded hostfreq function`	+12/-102
hostfreq_darwin.go `Add Darwin-specific cgo bridge for IOReport sampling`	+130/-0
hostfreq_other.go `Add stub implementation for non-Darwin platforms`	+12/-0
hostfreq_darwin.mm `Convert standalone binary to cgo library functions`	+200/-107
hostfreq_bridge.h `Add C header for cgo bridge interface`	+29/-0
hostfreq_darwin.cc `Add C++ wrapper for Objective-C++ implementation`	+1/-0

Tests

1 files

collector_test.go
Update tests for new hostfreq collector interface +4/-16

Configuration changes

11 files

host-install-macos.sh `Remove hostfreq binary installation and launchd service`	+0/-11
package-host-macos.sh `Update packaging to exclude hostfreq binary`	+1/-8
.bazelrc `Update platform configuration paths`	+6/-5
clang-tidy.yml `Update workflow to target cpufreq package`	+4/-2
Makefile `Add hostfreq object compilation and cgo build support`	+29/-22
BUILD.bazel `Remove hostfreq binary build target`	+0/-27
Makefile `Remove hostfreq binary build rules`	+0/-17
com.serviceradar.hostfreq.plist `Remove hostfreq launchd service configuration`	+0/-36
com.serviceradar.sysmonvm.plist `Remove hostfreq environment variable from checker service`	+0/-6
BUILD.bazel `Remove hostfreq binary from packaging targets`	+0/-32
BUILD.bazel `Add cgo library build configuration for Darwin`	+29/-0

Documentation

4 files

README.md `Update documentation for embedded sampler approach`	+6/-7
README.md `Update documentation for integrated approach`	+20/-56
cpu_plan.md `Update documentation for embedded sampler strategy`	+2/-2
sysmonvm-e2e.md `Update runbook for simplified deployment process`	+6/-7

Imported from GitHub pull request. Original GitHub pull request: #1746 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1746 Original created: 2025-10-12T06:44:32Z Original updated: 2025-10-12T17:59:59Z Original head: carverauto/serviceradar:sysmonvm-pkg-refresh Original base: main Original merged: 2025-10-12T17:59:56Z by @mfreeman451 --- ### **PR Type** Enhancement ___ ### **Description** - Embed macOS IOReport frequency sampler into Go binary - Remove standalone hostfreq binary and launchd service - Refactor cpufreq collector with cgo bridge - Update build system and packaging scripts ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Standalone hostfreq binary"] --> B["Embedded cgo sampler"] C["External launchd service"] --> D["Single checker process"] E["Command execution"] --> F["Direct C++ bridge"] B --> G["Simplified deployment"] D --> G F --> G ``` <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><details><summary>6 files</summary><table> <tr> <td><strong>collector.go</strong><dd><code>Refactor collector to use embedded hostfreq function</code>          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-35e6ecb698e97eb56fac27c2c42a1cb402cb477e408fc5bf08d7a7b233241b55">+12/-102</a></td> </tr> <tr> <td><strong>hostfreq_darwin.go</strong><dd><code>Add Darwin-specific cgo bridge for IOReport sampling</code>          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-122a8c8a7dd762a68dcd190e7ae30e13b5ba72c839951b577d7c17266fcb5483">+130/-0</a>  </td> </tr> <tr> <td><strong>hostfreq_other.go</strong><dd><code>Add stub implementation for non-Darwin platforms</code>                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-bd792010132c7a5530bb5e32b496b436107900269575d7d8ad5ee742c2bf33a9">+12/-0</a>    </td> </tr> <tr> <td><strong>hostfreq_darwin.mm</strong><dd><code>Convert standalone binary to cgo library functions</code>              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-efc7800c1f9935774e2a750a8d729c8be5e59d22fdc76c5d534d59148520b770">+200/-107</a></td> </tr> <tr> <td><strong>hostfreq_bridge.h</strong><dd><code>Add C header for cgo bridge interface</code>                                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-787a25d2cd7286a58fbcd38d453227be0c4813748048c640c5a7d2c019ee390e">+29/-0</a>    </td> </tr> <tr> <td><strong>hostfreq_darwin.cc</strong><dd><code>Add C++ wrapper for Objective-C++ implementation</code>                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-a52d886afe4a6ce2de3a8f42b4925518f485e5f5114233bb1daf287adf69a0f5">+1/-0</a>      </td> </tr> </table></details></td></tr><tr><td><strong>Tests</strong></td><td><details><summary>1 files</summary><table> <tr> <td><strong>collector_test.go</strong><dd><code>Update tests for new hostfreq collector interface</code>                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-e533eef8916b184b7151d26c7d97b6375e81bb6b742b671e6f509024d8add521">+4/-16</a>    </td> </tr> </table></details></td></tr><tr><td><strong>Configuration changes</strong></td><td><details><summary>11 files</summary><table> <tr> <td><strong>host-install-macos.sh</strong><dd><code>Remove hostfreq binary installation and launchd service</code>    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-b8b0940137a4ea6f6800b649649c83c52dea5d1c59c3a7e5baa5ca20405eeb54">+0/-11</a>    </td> </tr> <tr> <td><strong>package-host-macos.sh</strong><dd><code>Update packaging to exclude hostfreq binary</code>                            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-5b027d22a6b110520421c7011cf90654b8fe427a5def3fae7b48c749c9b0402e">+1/-8</a>      </td> </tr> <tr> <td><strong>.bazelrc</strong><dd><code>Update platform configuration paths</code>                                            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-544556920c45b42cbfe40159b082ce8af6bd929e492d076769226265f215832f">+6/-5</a>      </td> </tr> <tr> <td><strong>clang-tidy.yml</strong><dd><code>Update workflow to target cpufreq package</code>                                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-2b7b9696b464e236f8066c1c1d33c6c91c95160f813e2b3fe59ddf0e83057ebf">+4/-2</a>      </td> </tr> <tr> <td><strong>Makefile</strong><dd><code>Add hostfreq object compilation and cgo build support</code>        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52">+29/-22</a>  </td> </tr> <tr> <td><strong>BUILD.bazel</strong><dd><code>Remove hostfreq binary build target</code>                                            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-184fb3e268873ce89047d398ddf2b684f39118babe9b0ca9c5075416aabdd506">+0/-27</a>    </td> </tr> <tr> <td><strong>Makefile</strong><dd><code>Remove hostfreq binary build rules</code>                                              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-1341584ba45b89b59c3ad200da555ca9645bde184d88b775ded625f9b9ae0137">+0/-17</a>    </td> </tr> <tr> <td><strong>com.serviceradar.hostfreq.plist</strong><dd><code>Remove hostfreq launchd service configuration</code>                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-010f443f7026527a65d5a84cbbb773a9f12bc567363a7ac839d37f3e859007e4">+0/-36</a>    </td> </tr> <tr> <td><strong>com.serviceradar.sysmonvm.plist</strong><dd><code>Remove hostfreq environment variable from checker service</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-f4bd5e4e0b6397df1c8eadf38700b76ded7bfc8e698d5fc3fcef159a63e8300f">+0/-6</a>      </td> </tr> <tr> <td><strong>BUILD.bazel</strong><dd><code>Remove hostfreq binary from packaging targets</code>                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-61bb5e1d8b1e798d3a834a32ab46845f70bd9bd4b7d20bbfd76a8016e34f9754">+0/-32</a>    </td> </tr> <tr> <td><strong>BUILD.bazel</strong><dd><code>Add cgo library build configuration for Darwin</code>                      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-eae467757a0a2497ce885e674634590de3a3ba84df236482436984f143fe45e0">+29/-0</a>    </td> </tr> </table></details></td></tr><tr><td><strong>Documentation</strong></td><td><details><summary>4 files</summary><table> <tr> <td><strong>README.md</strong><dd><code>Update documentation for embedded sampler approach</code>              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-f72d7c40673ad7eb78ef313ce5aa05442176de542f531a96ca5776156ac41d4b">+6/-7</a>      </td> </tr> <tr> <td><strong>README.md</strong><dd><code>Update documentation for integrated approach</code>                          </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-aa494b7da7f57782ef755e899ddfd8f8c3ec6fe980604a57bf98db5d3c8f611c">+20/-56</a>  </td> </tr> <tr> <td><strong>cpu_plan.md</strong><dd><code>Update documentation for embedded sampler strategy</code>              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-c0dc94ab36dba7fcd11ecff636aa9814b5a34f935329473e01dc648a84ea8cb1">+2/-2</a>      </td> </tr> <tr> <td><strong>sysmonvm-e2e.md</strong><dd><code>Update runbook for simplified deployment process</code>                  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-43e6004242d0658c1e3f03e280f6b40e67df1d60345387a6a6863ef341b5ceb1">+6/-7</a>      </td> </tr> </table></details></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-10-12 06:45:16 +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/1746#issuecomment-3393993000
Original created: 2025-10-12T06:45:16Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Native memory leak Description: C-allocated buffers are returned to Go via cgo (using malloc) and require explicit freeing; if Go code does not always call `hostfreq_free` on both `outJSON` and `outError`, it can leak memory across repeated calls. hostfreq_darwin.mm [481-486] Referred Code `extern "C" void hostfreq_free(char* ptr) { if (ptr != nullptr) { free(ptr); } }`
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
⚪	No custom compliance provided Follow the guide to enable custom compliance check.
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/1746#issuecomment-3393993000 Original created: 2025-10-12T06:45:16Z --- ## 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 rowspan=1>⚪</td> <td><details><summary><strong>Native memory leak </strong></summary><br> <b>Description:</b> C-allocated buffers are returned to Go via cgo (using malloc) and require explicit <br>freeing; if Go code does not always call <code>hostfreq_free</code> on both <code>outJSON</code> and <code>outError</code>, it <br>can leak memory across repeated calls.<br> <strong><a href='https://github.com/carverauto/serviceradar/pull/1746/files#diff-efc7800c1f9935774e2a750a8d729c8be5e59d22fdc76c5d534d59148520b770R481-R486'>hostfreq_darwin.mm [481-486]</a></strong><br> <details open><summary>Referred Code</summary> ```objective-c++ extern "C" void hostfreq_free(char* ptr) { if (ptr != nullptr) { free(ptr); } } ``` </details></details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary>🎫 <strong>No ticket provided </summary></strong> - [ ] Create ticket/issue  </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>⚪</td><td><details><summary><strong>No custom compliance provided</strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/'>guide</a> to enable custom compliance check. </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-10-12 06:46:41 +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/1746#issuecomment-3393993829
Original created: 2025-10-12T06:46:41Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Simplify the cgo build process Unify the build process by removing the manual `Makefile` pre-compilation of the Objective-C++ object file. Instead, rely solely on standard `cgo` directives to let the Go toolchain manage the compilation, simplifying the build logic and eliminating the need for a dual build system with Bazel. Examples: Makefile [76-86] `ifeq ($(HOST_OS),Darwin) HOSTFREQ_OBJ := pkg/cpufreq/hostfreq_darwin_embed.o HOSTFREQ_SRC := pkg/cpufreq/hostfreq_darwin.mm HOSTFREQ_HDR := pkg/cpufreq/hostfreq_bridge.h $(HOSTFREQ_OBJ): $(HOSTFREQ_SRC) $(HOSTFREQ_HDR) @echo "$(COLOR_BOLD)Compiling hostfreq Objective-C++ bridge$(COLOR_RESET)" @xcrun clang++ -arch arm64 -std=c++20 -fobjc-arc -x objective-c++ -I pkg/cpufreq -c $(HOSTFREQ_SRC) -o $@ .PHONY: hostfreq-embed-object ... (clipped 1 lines)` pkg/cpufreq/hostfreq_darwin.go [1-11] `//go:build darwin package cpufreq /* #cgo darwin CFLAGS: -fobjc-arc #cgo darwin CXXFLAGS: -std=c++20 -fobjc-arc -x objective-c++ #cgo darwin LDFLAGS: -framework Foundation -framework IOKit -framework CoreFoundation -lIOReport #include "hostfreq_bridge.h" / ... (clipped 1 lines)` Solution Walkthrough: Before: # Makefile HOSTFREQ_OBJ := pkg/cpufreq/hostfreq_darwin_embed.o HOSTFREQ_SRC := pkg/cpufreq/hostfreq_darwin.mm $(HOSTFREQ_OBJ): $(HOSTFREQ_SRC) # Manually compile Objective-C++ to an object file @xcrun clang++ ... -c $(HOSTFREQ_SRC) -o $@ sysmonvm-build-checker-darwin: hostfreq-embed-object # Build Go binary, assuming the object file exists GOOS=darwin GOARCH=arm64 CGO_ENABLED=1 go build -tags hostfreq_embed ... # pkg/cpufreq/hostfreq_darwin.go //go:build darwin && hostfreq_embed // This build tag is used to link the pre-compiled object file After: # Makefile # Remove manual Objective-C++ compilation steps # HOSTFREQ_OBJ, hostfreq-embed-object targets are gone sysmonvm-build-checker-darwin: # Let the Go toolchain handle the entire cgo build GOOS=darwin GOARCH=arm64 CGO_ENABLED=1 go build ... # pkg/cpufreq/hostfreq_darwin.go //go:build darwin / #cgo CXXFLAGS: -std=c++20 -fobjc-arc -x objective-c++ #cgo LDFLAGS: -framework Foundation -framework IOKit ... #include "hostfreq_bridge.h" */ import "C" // No more `hostfreq_embed` build tag needed Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a significant design flaw where two separate and complex build systems (`make` and `Bazel`) are used for the same cgo component, increasing maintenance overhead and fragility. Unifying the build process would be a major improvement.	Medium
Possible issue	Make CGo call respect context Refactor `collectViaHostfreq` to run the blocking CGo call in a separate goroutine, allowing the function to respect context cancellation via a `select` statement. pkg/cpufreq/hostfreq_darwin.go [44-130] func collectViaHostfreq(ctx context.Context, window time.Duration) (Snapshot, error) { - select { - case <-ctx.Done(): - return nil, ctx.Err() - default: - } - interval := int(window / time.Millisecond) if interval <= 0 { interval = int(defaultSampleWindow / time.Millisecond) } - var outJSON C.char - var outError C.char - var actual C.double + type result struct { + status int + outJSON C.char + outError C.char + } - const smoothingEnabled = 1 - //nolint:gocritic // false positive when analyzing CGO bridge macro. - status := int(C.hostfreq_collect_json( - C.int(interval), - C.int(smoothingEnabled), - &outJSON, - &actual, - &outError, - )) - defer C.hostfreq_free(outJSON) - defer C.hostfreq_free(outError) + resultChan := make(chan result, 1) - if status != hostfreqStatusOK { - message := "" - if outError != nil { - message = C.GoString(outError) - } - if message == "" { - message = C.GoString(C.hostfreq_status_string(C.int(status))) + go func() { + var res result + const smoothingEnabled = 1 + //nolint:gocritic // false positive when analyzing CGO bridge macro. + res.status = int(C.hostfreq_collect_json( + C.int(interval), + C.int(smoothingEnabled), + &res.outJSON, + nil, // actual interval not used + &res.outError, + )) + resultChan <- res + }() + + select { + case <-ctx.Done(): + return nil, ctx.Err() + case res := <-resultChan: + defer C.hostfreq_free(res.outJSON) + defer C.hostfreq_free(res.outError) + + if res.status != hostfreqStatusOK { + message := "" + if res.outError != nil { + message = C.GoString(res.outError) + } + if message == "" { + message = C.GoString(C.hostfreq_status_string(C.int(res.status))) + } + + switch res.status { + case hostfreqStatusUnavailable: + if message != "" { + return nil, fmt.Errorf("%w: %s", ErrFrequencyUnavailable, message) + } + return nil, ErrFrequencyUnavailable + case hostfreqStatusPermission: + return nil, fmt.Errorf("%w: %s", errHostfreqPermission, message) + default: + return nil, fmt.Errorf("%w: %s", errHostfreqInternal, message) + } } - switch status { - case hostfreqStatusUnavailable: - if message != "" { - return nil, fmt.Errorf("%w: %s", ErrFrequencyUnavailable, message) - } - return nil, ErrFrequencyUnavailable - case hostfreqStatusPermission: - return nil, fmt.Errorf("%w: %s", errHostfreqPermission, message) - default: - return nil, fmt.Errorf("%w: %s", errHostfreqInternal, message) - } - } - - if outJSON == nil { - return nil, errHostfreqNoData - } - - jsonStr := C.GoString(outJSON) - if jsonStr == "" { - return nil, errHostfreqEmptyPayload - } - - var payload hostfreqPayload - if err := json.Unmarshal([]byte(jsonStr), &payload); err != nil { - return nil, fmt.Errorf("failed to parse hostfreq output: %w", err) - } - - if len(payload.Cores) == 0 { - return nil, ErrFrequencyUnavailable - } - - snapshot := &Snapshot{ - Cores: make([]CoreFrequency, 0, len(payload.Cores)), - } - - for idx, core := range payload.Cores { - hz := core.AvgMHz 1_000_000 - if hz < 0 { - hz = 0 + if res.outJSON == nil { + return nil, errHostfreqNoData } - snapshot.Cores = append(snapshot.Cores, CoreFrequency{ - CoreID: idx, - FrequencyHz: hz, - Source: FrequencySourceHostfreq, - }) + jsonStr := C.GoString(res.outJSON) + if jsonStr == "" { + return nil, errHostfreqEmptyPayload + } + + var payload hostfreqPayload + if err := json.Unmarshal([]byte(jsonStr), &payload); err != nil { + return nil, fmt.Errorf("failed to parse hostfreq output: %w", err) + } + + if len(payload.Cores) == 0 { + return nil, ErrFrequencyUnavailable + } + + snapshot := &Snapshot{ + Cores: make([]CoreFrequency, 0, len(payload.Cores)), + } + + for idx, core := range payload.Cores { + hz := core.AvgMHz * 1_000_000 + if hz < 0 { + hz = 0 + } + + snapshot.Cores = append(snapshot.Cores, CoreFrequency{ + CoreID: idx, + FrequencyHz: hz, + Source: FrequencySourceHostfreq, + }) + } + + return snapshot, nil } - - return snapshot, nil } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies that the blocking CGo call does not respect context cancellation and proposes a standard Go pattern to fix it, which prevents potential goroutine leaks and makes the function properly cancellable.	Medium
Possible issue	Fix a memory leak Add a call to `CFRelease(subscription)` in an error path within `CollectJsonInternal` to prevent a memory leak if `IOReportCreateSubscription` partially fails. pkg/cpufreq/hostfreq_darwin.mm [333-417] Status CollectJsonInternal(const CollectorConfig& config, std::string* json, double* actualDurationMs, std::string* error) { std::vector<double> ecpuTable = LoadDvfsTable(CFSTR("voltage-states1-sram")); std::vector<double> pcpuTable = LoadDvfsTable(CFSTR("voltage-states5-sram")); if (ecpuTable.size() <= 1 && pcpuTable.size() <= 1) { AppendError(error, "Failed to read DVFS tables from IORegistry. Run on Apple Silicon macOS with sudo."); return Status::kUnavailable; } CFMutableDictionaryRef subscribedChannels = nullptr; CFMutableDictionaryRef cpuChannels = IOReportCopyChannelsInGroup(@"CPU Stats", nil, 0, 0, 0); if (!cpuChannels) { AppendError(error, "IOReportCopyChannelsInGroup(\"CPU Stats\") returned null."); return Status::kPermission; } IOReportSubscriptionRef subscription = IOReportCreateSubscription(nullptr, cpuChannels, &subscribedChannels, 0, nullptr); CFRelease(cpuChannels); if (!subscription \|\| !subscribedChannels) { AppendError(error, "IOReportCreateSubscription failed. Try running as a privileged user."); if (subscribedChannels) { CFRelease(subscribedChannels); + } + if (subscription) { + CFRelease(subscription); } return Status::kPermission; } auto cleanup = [&]() { if (subscribedChannels) { CFRelease(subscribedChannels); } if (subscription) { CFRelease(subscription); } }; auto emitSample = [&]() -> Status { double actualDuration = 0.0; auto result = CollectFrequencies(subscription, subscribedChannels, ecpuTable, pcpuTable, config, &actualDuration, error); if (!result.has_value()) { return Status::kInternal; } auto jsonOutput = EncodeJson(config, actualDuration, result->first, result->second, error); if (!jsonOutput.has_value()) { return Status::kInternal; } if (actualDurationMs) { actualDurationMs = actualDuration; } if (json) { json = std::move(jsonOutput); } return Status::kOk; }; Status status = Status::kOk; for (int sampleIndex = 0; sampleIndex < config.sampleCount; ++sampleIndex) { status = emitSample(); if (status != Status::kOk) { break; } } cleanup(); return status; } Apply / Chat* Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a memory leak in an error path for a newly added C++ function and provides the correct fix, improving the robustness of the code.	Medium
Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1746#issuecomment-3393993829 Original created: 2025-10-12T06:46:41Z --- ## 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>High-level</td> <td> <details><summary>Simplify the cgo build process</summary> ___ **Unify the build process by removing the manual <code>Makefile</code> pre-compilation of the <br>Objective-C++ object file. Instead, rely solely on standard <code>cgo</code> directives to <br>let the Go toolchain manage the compilation, simplifying the build logic and <br>eliminating the need for a dual build system with Bazel.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-76ed074a9305c04054cdebb9e9aad2d818052b07091de1f20cad0bbac34ffb52R76-R86">Makefile [76-86]</a> </summary> ```makefile ifeq ($(HOST_OS),Darwin) HOSTFREQ_OBJ := pkg/cpufreq/hostfreq_darwin_embed.o HOSTFREQ_SRC := pkg/cpufreq/hostfreq_darwin.mm HOSTFREQ_HDR := pkg/cpufreq/hostfreq_bridge.h $(HOSTFREQ_OBJ): $(HOSTFREQ_SRC) $(HOSTFREQ_HDR) @echo "$(COLOR_BOLD)Compiling hostfreq Objective-C++ bridge$(COLOR_RESET)" @xcrun clang++ -arch arm64 -std=c++20 -fobjc-arc -x objective-c++ -I pkg/cpufreq -c $(HOSTFREQ_SRC) -o $@ .PHONY: hostfreq-embed-object ... (clipped 1 lines) ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1746/files#diff-122a8c8a7dd762a68dcd190e7ae30e13b5ba72c839951b577d7c17266fcb5483R1-R11">pkg/cpufreq/hostfreq_darwin.go [1-11]</a> </summary> ```go //go:build darwin package cpufreq /* #cgo darwin CFLAGS: -fobjc-arc #cgo darwin CXXFLAGS: -std=c++20 -fobjc-arc -x objective-c++ #cgo darwin LDFLAGS: -framework Foundation -framework IOKit -framework CoreFoundation -lIOReport #include "hostfreq_bridge.h" */ ... (clipped 1 lines) ``` </details> ### Solution Walkthrough: #### Before: ```go # Makefile HOSTFREQ_OBJ := pkg/cpufreq/hostfreq_darwin_embed.o HOSTFREQ_SRC := pkg/cpufreq/hostfreq_darwin.mm $(HOSTFREQ_OBJ): $(HOSTFREQ_SRC) # Manually compile Objective-C++ to an object file @xcrun clang++ ... -c $(HOSTFREQ_SRC) -o $@ sysmonvm-build-checker-darwin: hostfreq-embed-object # Build Go binary, assuming the object file exists GOOS=darwin GOARCH=arm64 CGO_ENABLED=1 go build -tags hostfreq_embed ... # pkg/cpufreq/hostfreq_darwin.go //go:build darwin && hostfreq_embed // This build tag is used to link the pre-compiled object file ``` #### After: ```go # Makefile # Remove manual Objective-C++ compilation steps # HOSTFREQ_OBJ, hostfreq-embed-object targets are gone sysmonvm-build-checker-darwin: # Let the Go toolchain handle the entire cgo build GOOS=darwin GOARCH=arm64 CGO_ENABLED=1 go build ... # pkg/cpufreq/hostfreq_darwin.go //go:build darwin /* #cgo CXXFLAGS: -std=c++20 -fobjc-arc -x objective-c++ #cgo LDFLAGS: -framework Foundation -framework IOKit ... #include "hostfreq_bridge.h" */ import "C" // No more `hostfreq_embed` build tag needed ``` <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies a significant design flaw where two separate and complex build systems (`make` and `Bazel`) are used for the same cgo component, increasing maintenance overhead and fragility. Unifying the build process would be a major improvement. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=2>Possible issue</td> <td> <details><summary>Make CGo call respect context</summary> ___ **Refactor <code>collectViaHostfreq</code> to run the blocking CGo call in a separate <br>goroutine, allowing the function to respect context cancellation via a <code>select</code> <br>statement.** [pkg/cpufreq/hostfreq_darwin.go [44-130]](https://github.com/carverauto/serviceradar/pull/1746/files#diff-122a8c8a7dd762a68dcd190e7ae30e13b5ba72c839951b577d7c17266fcb5483R44-R130) ```diff func collectViaHostfreq(ctx context.Context, window time.Duration) (*Snapshot, error) { - select { - case <-ctx.Done(): - return nil, ctx.Err() - default: - } - interval := int(window / time.Millisecond) if interval <= 0 { interval = int(defaultSampleWindow / time.Millisecond) } - var outJSON *C.char - var outError *C.char - var actual C.double + type result struct { + status int + outJSON *C.char + outError *C.char + } - const smoothingEnabled = 1 - //nolint:gocritic // false positive when analyzing CGO bridge macro. - status := int(C.hostfreq_collect_json( - C.int(interval), - C.int(smoothingEnabled), - &outJSON, - &actual, - &outError, - )) - defer C.hostfreq_free(outJSON) - defer C.hostfreq_free(outError) + resultChan := make(chan result, 1) - if status != hostfreqStatusOK { - message := "" - if outError != nil { - message = C.GoString(outError) - } - if message == "" { - message = C.GoString(C.hostfreq_status_string(C.int(status))) + go func() { + var res result + const smoothingEnabled = 1 + //nolint:gocritic // false positive when analyzing CGO bridge macro. + res.status = int(C.hostfreq_collect_json( + C.int(interval), + C.int(smoothingEnabled), + &res.outJSON, + nil, // actual interval not used + &res.outError, + )) + resultChan <- res + }() + + select { + case <-ctx.Done(): + return nil, ctx.Err() + case res := <-resultChan: + defer C.hostfreq_free(res.outJSON) + defer C.hostfreq_free(res.outError) + + if res.status != hostfreqStatusOK { + message := "" + if res.outError != nil { + message = C.GoString(res.outError) + } + if message == "" { + message = C.GoString(C.hostfreq_status_string(C.int(res.status))) + } + + switch res.status { + case hostfreqStatusUnavailable: + if message != "" { + return nil, fmt.Errorf("%w: %s", ErrFrequencyUnavailable, message) + } + return nil, ErrFrequencyUnavailable + case hostfreqStatusPermission: + return nil, fmt.Errorf("%w: %s", errHostfreqPermission, message) + default: + return nil, fmt.Errorf("%w: %s", errHostfreqInternal, message) + } } - switch status { - case hostfreqStatusUnavailable: - if message != "" { - return nil, fmt.Errorf("%w: %s", ErrFrequencyUnavailable, message) - } - return nil, ErrFrequencyUnavailable - case hostfreqStatusPermission: - return nil, fmt.Errorf("%w: %s", errHostfreqPermission, message) - default: - return nil, fmt.Errorf("%w: %s", errHostfreqInternal, message) - } - } - - if outJSON == nil { - return nil, errHostfreqNoData - } - - jsonStr := C.GoString(outJSON) - if jsonStr == "" { - return nil, errHostfreqEmptyPayload - } - - var payload hostfreqPayload - if err := json.Unmarshal([]byte(jsonStr), &payload); err != nil { - return nil, fmt.Errorf("failed to parse hostfreq output: %w", err) - } - - if len(payload.Cores) == 0 { - return nil, ErrFrequencyUnavailable - } - - snapshot := &Snapshot{ - Cores: make([]CoreFrequency, 0, len(payload.Cores)), - } - - for idx, core := range payload.Cores { - hz := core.AvgMHz * 1_000_000 - if hz < 0 { - hz = 0 + if res.outJSON == nil { + return nil, errHostfreqNoData } - snapshot.Cores = append(snapshot.Cores, CoreFrequency{ - CoreID: idx, - FrequencyHz: hz, - Source: FrequencySourceHostfreq, - }) + jsonStr := C.GoString(res.outJSON) + if jsonStr == "" { + return nil, errHostfreqEmptyPayload + } + + var payload hostfreqPayload + if err := json.Unmarshal([]byte(jsonStr), &payload); err != nil { + return nil, fmt.Errorf("failed to parse hostfreq output: %w", err) + } + + if len(payload.Cores) == 0 { + return nil, ErrFrequencyUnavailable + } + + snapshot := &Snapshot{ + Cores: make([]CoreFrequency, 0, len(payload.Cores)), + } + + for idx, core := range payload.Cores { + hz := core.AvgMHz * 1_000_000 + if hz < 0 { + hz = 0 + } + + snapshot.Cores = append(snapshot.Cores, CoreFrequency{ + CoreID: idx, + FrequencyHz: hz, + Source: FrequencySourceHostfreq, + }) + } + + return snapshot, nil } - - return snapshot, nil } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies that the blocking CGo call does not respect context cancellation and proposes a standard Go pattern to fix it, which prevents potential goroutine leaks and makes the function properly cancellable. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Fix a memory leak</summary> ___ **Add a call to <code>CFRelease(subscription)</code> in an error path within <br><code>CollectJsonInternal</code> to prevent a memory leak if <code>IOReportCreateSubscription</code> <br>partially fails.** [pkg/cpufreq/hostfreq_darwin.mm [333-417]](https://github.com/carverauto/serviceradar/pull/1746/files#diff-efc7800c1f9935774e2a750a8d729c8be5e59d22fdc76c5d534d59148520b770R333-R417) ```diff Status CollectJsonInternal(const CollectorConfig& config, std::string* json, double* actualDurationMs, std::string* error) { std::vector<double> ecpuTable = LoadDvfsTable(CFSTR("voltage-states1-sram")); std::vector<double> pcpuTable = LoadDvfsTable(CFSTR("voltage-states5-sram")); if (ecpuTable.size() <= 1 && pcpuTable.size() <= 1) { AppendError(error, "Failed to read DVFS tables from IORegistry. Run on Apple Silicon macOS with sudo."); return Status::kUnavailable; } CFMutableDictionaryRef subscribedChannels = nullptr; CFMutableDictionaryRef cpuChannels = IOReportCopyChannelsInGroup(@"CPU Stats", nil, 0, 0, 0); if (!cpuChannels) { AppendError(error, "IOReportCopyChannelsInGroup(\"CPU Stats\") returned null."); return Status::kPermission; } IOReportSubscriptionRef subscription = IOReportCreateSubscription(nullptr, cpuChannels, &subscribedChannels, 0, nullptr); CFRelease(cpuChannels); if (!subscription || !subscribedChannels) { AppendError(error, "IOReportCreateSubscription failed. Try running as a privileged user."); if (subscribedChannels) { CFRelease(subscribedChannels); + } + if (subscription) { + CFRelease(subscription); } return Status::kPermission; } auto cleanup = [&]() { if (subscribedChannels) { CFRelease(subscribedChannels); } if (subscription) { CFRelease(subscription); } }; auto emitSample = [&]() -> Status { double actualDuration = 0.0; auto result = CollectFrequencies(subscription, subscribedChannels, ecpuTable, pcpuTable, config, &actualDuration, error); if (!result.has_value()) { return Status::kInternal; } auto jsonOutput = EncodeJson(config, actualDuration, result->first, result->second, error); if (!jsonOutput.has_value()) { return Status::kInternal; } if (actualDurationMs) { *actualDurationMs = actualDuration; } if (json) { *json = std::move(*jsonOutput); } return Status::kOk; }; Status status = Status::kOk; for (int sampleIndex = 0; sampleIndex < config.sampleCount; ++sampleIndex) { status = emitSample(); if (status != Status::kOk) { break; } } cleanup(); return status; } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies a memory leak in an error path for a newly added C++ function and provides the correct fix, improving the robustness of the code. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>