carverauto/serviceradar

Fork 0

db pkg cleanup #2415

Merged

mfreeman451 merged 1 commit from refs/pull/2415/head into main

2025-11-16 17:02:55 +00:00

mfreeman451 commented

2025-11-16 16:54:23 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #1946
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1946
Original created: 2025-11-16T16:54:23Z
Original updated: 2025-11-16T17:03:15Z
Original head: carverauto/serviceradar:updates/db_cleanup
Original base: main
Original merged: 2025-11-16T17:02:55Z 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

Enhancement, Bug fix

Description

Remove Proton compatibility shim (db.Conn, CompatConn) from database layer
Migrate all registry queries to native CNPG/pgx with $n placeholders
Add typed ServiceRegistrationEvent struct and batch insert helper
Update sysmon API queries to use QueryCNPGRows and UTC timestamp normalization
Simplify registry operations by eliminating placeholder rewriting logic

Diagram Walkthrough

flowchart LR
  A["pkg/registry<br/>Query/Insert calls"] -->|"Previously via<br/>db.Conn shim"| B["CompatConn<br/>Placeholder rewrite"]
  B --> C["pgxpool"]
  A -->|"Now direct"| D["QueryCNPGRows<br/>ExecCNPG<br/>InsertServiceRegistrationEvents"]
  D --> C
  E["ServiceRegistrationEvent<br/>struct"] --> D
  F["Sysmon API<br/>metrics queries"] -->|"Updated to<br/>QueryCNPGRows"| D

File Walkthrough

Relevant files

Enhancement

6 files

sysmon.go `Replace direct Conn.Query with QueryCNPGRows helper`	+10/-10
cnpg_registry.go `Add ServiceRegistrationEvent type and batch insert`	+84/-0
errors.go `Remove compat connection errors, add event validation error`	+3/-7
registry.go `Replace QueryCNPGRows with QueryRegistryRows method`	+2/-2
service_registry.go `Migrate event emission and deletion to native CNPG`	+112/-111
service_registry_queries.go `Remove Proton-style queries, use CNPG helpers directly`	+16/-521

Tests

3 files

cnpg_registry_test.go `Test ServiceRegistrationEvent argument building`	+37/-0
registry_cnpg_test.go `Update mock to use QueryRegistryRows interface`	+1/-1
service_registry_queries_test.go `Remove placeholder escaping tests, add event writer tests`	+140/-70

Bug fix

1 files

db.go
Remove CompatConn and placeholder rewriting logic +5/-184

Documentation

7 files

interfaces.go `Update Service interface documentation for CNPG`	+1/-1
FRICTION_POINTS.md `Update DELETE example to use ExecCNPG`	+4/-2
cnpg-monitoring.md `Add CNPG monitoring dashboards and retention checks`	+130/-0
kv-ci-plan.md `Add KV regression safeguards and CI validation plan`	+58/-0
proposal.md `Document rationale for removing Proton compatibility layer`	+17/-0
spec.md `Define requirements for Postgres-native registry tables`	+13/-0
tasks.md `Track completion of registry compatibility removal tasks`	+9/-0

Imported from GitHub pull request. Original GitHub pull request: #1946 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1946 Original created: 2025-11-16T16:54:23Z Original updated: 2025-11-16T17:03:15Z Original head: carverauto/serviceradar:updates/db_cleanup Original base: main Original merged: 2025-11-16T17:02:55Z 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** Enhancement, Bug fix ___ ### **Description** - Remove Proton compatibility shim (`db.Conn`, `CompatConn`) from database layer - Migrate all registry queries to native CNPG/pgx with `$n` placeholders - Add typed `ServiceRegistrationEvent` struct and batch insert helper - Update sysmon API queries to use `QueryCNPGRows` and UTC timestamp normalization - Simplify registry operations by eliminating placeholder rewriting logic ___ ### Diagram Walkthrough ```mermaid flowchart LR A["pkg/registry Query/Insert calls"] -->|"Previously via db.Conn shim"| B["CompatConn Placeholder rewrite"] B --> C["pgxpool"] A -->|"Now direct"| D["QueryCNPGRows ExecCNPG InsertServiceRegistrationEvents"] D --> C E["ServiceRegistrationEvent struct"] --> D F["Sysmon API metrics queries"] -->|"Updated to QueryCNPGRows"| D ``` <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><details><summary>6 files</summary><table> <tr> <td>sysmon.go<dd><code>Replace direct Conn.Query with QueryCNPGRows helper</code>            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1946/files#diff-4879e8b367ee55413c6cb006f23bb912261df5310c640d3af352033cf97ffc84">+10/-10</a>  </td> </tr> <tr> <td>cnpg_registry.go<dd><code>Add ServiceRegistrationEvent type and batch insert</code>              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1946/files#diff-e31b8b854d9ba774d2f3ed9899b1f5902462cd0e63990a2898dcaf9bc171d571">+84/-0</a>    </td> </tr> <tr> <td>errors.go<dd><code>Remove compat connection errors, add event validation error</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1946/files#diff-993d3cbb9129e2dbe72c2785718929ee4a7dc9e3974b73f717f8c83eede44957">+3/-7</a>      </td> </tr> <tr> <td>registry.go<dd><code>Replace QueryCNPGRows with QueryRegistryRows method</code>            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1946/files#diff-cb61d8f79451b9541de4a8cc0811523a68d15452b2f5971c7618ea5b423cf4ec">+2/-2</a>      </td> </tr> <tr> <td>service_registry.go<dd><code>Migrate event emission and deletion to native CNPG</code>              </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1946/files#diff-d6d6d09a58edde934a1d3f571ff5ce02f0475c5e85ecdec27888892aff8d9d1d">+112/-111</a></td> </tr> <tr> <td>service_registry_queries.go<dd><code>Remove Proton-style queries, use CNPG helpers directly</code>      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1946/files#diff-3591e8646384be68811f862b986342253cabc23176c6f0e09996453baf88a2e9">+16/-521</a></td> </tr> </table></details></td></tr><tr><td>Tests</td><td><details><summary>3 files</summary><table> <tr> <td>cnpg_registry_test.go<dd><code>Test ServiceRegistrationEvent argument building</code>                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1946/files#diff-e38716ee5b3614089e00e851d22d2c61e9934ebcc2b44b7aa4b79623251af869">+37/-0</a>    </td> </tr> <tr> <td>registry_cnpg_test.go<dd><code>Update mock to use QueryRegistryRows interface</code>                      </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1946/files#diff-175c7dc3d844415bcdce3b7654e947aec09346eee22ac4e67487b0b491d46e21">+1/-1</a>      </td> </tr> <tr> <td>service_registry_queries_test.go<dd><code>Remove placeholder escaping tests, add event writer tests</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1946/files#diff-6375daf4a77f18366326db1ac818f613eea7735f4bdc23c886274edc58826656">+140/-70</a></td> </tr> </table></details></td></tr><tr><td>Bug fix</td><td><details><summary>1 files</summary><table> <tr> <td>db.go<dd><code>Remove CompatConn and placeholder rewriting logic</code>                </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1946/files#diff-5da3684806835246d262230050593f460b12b6c0e3966df174e6061be0e9e575">+5/-184</a>  </td> </tr> </table></details></td></tr><tr><td>Documentation</td><td><details><summary>7 files</summary><table> <tr> <td>interfaces.go<dd><code>Update Service interface documentation for CNPG</code>                    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1946/files#diff-c230fe0c315251837357bfde4ae7f7b34080398d8e48af6bf78badb2124271f3">+1/-1</a>      </td> </tr> <tr> <td>FRICTION_POINTS.md<dd><code>Update DELETE example to use ExecCNPG</code>                                        </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1946/files#diff-b0653c58880f810ba832c0500733d63de309db98b43009fe73a1862494cf41bd">+4/-2</a>      </td> </tr> <tr> <td>cnpg-monitoring.md<dd><code>Add CNPG monitoring dashboards and retention checks</code>            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1946/files#diff-8c105f73fd3323743d06525e84c5d0c686b447742dca1a5cf2776b1afc9b8ba2">+130/-0</a>  </td> </tr> <tr> <td>kv-ci-plan.md<dd><code>Add KV regression safeguards and CI validation plan</code>            </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1946/files#diff-60ccc8f58261bec42e9c77f3647e9b40505e2c5c343b5de62add7ff22581c8e7">+58/-0</a>    </td> </tr> <tr> <td>proposal.md<dd><code>Document rationale for removing Proton compatibility layer</code></dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1946/files#diff-9ffe47187efe19b5084ed1a469b3968036d7e8ceaef769941d036d654b875bae">+17/-0</a>    </td> </tr> <tr> <td>spec.md<dd><code>Define requirements for Postgres-native registry tables</code>    </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1946/files#diff-d16036359c50038867c508ffd03d819586d7940f78c71e9e964608944b5fd7b4">+13/-0</a>    </td> </tr> <tr> <td>tasks.md<dd><code>Track completion of registry compatibility removal tasks</code>  </dd></td> <td><a href="https://github.com/carverauto/serviceradar/pull/1946/files#diff-a95a4231cd1c5a744e1cb535f65f8b5567c1a20090792590c4a573962521c4a3">+9/-0</a>      </td> </tr> </table></details></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-11-16 16:54:54 +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/1946#issuecomment-3538944722
Original created: 2025-11-16T16:54:54Z

PR Compliance Guide 🔍

(Compliance updated until commit `github.com/carverauto/serviceradar@1d9cc06b71`)

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: 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

Previous compliance checks

Compliance check up to commit 1d9cc06

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: 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

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1946#issuecomment-3538944722 Original created: 2025-11-16T16:54:54Z --- ## PR Compliance Guide 🔍  #### (Compliance updated until commit https://github.com/carverauto/serviceradar/commit/1d9cc06b712f768c7ccff4eed8183fdab90fc4d5) 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=6>🟢</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: Robust Error Handling and Edge Case Management</summary> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful degradation **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> <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:** 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 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> ___ #### Previous compliance checks <details> <summary>Compliance check up to commit <a href='https://github.com/carverauto/serviceradar/commit/1d9cc06b712f768c7ccff4eed8183fdab90fc4d5'>1d9cc06</a></summary> <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=6>🟢</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: Robust Error Handling and Edge Case Management</summary> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful degradation **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> <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:** 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 align="center" colspan="2">   </td></tr></tbody></table> </details>

qodo-code-review[bot] commented

2025-11-16 16:56: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/1946#issuecomment-3538945649
Original created: 2025-11-16T16:56:06Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Avoid N+1 queries during service purge Optimize `PurgeInactive` to avoid N+1 queries by creating a private helper function that performs the deletion directly, bypassing the redundant status checks in `DeleteService`. pkg/registry/service_registry.go [712-805] func (r ServiceRegistry) PurgeInactive(ctx context.Context, retentionPeriod time.Duration) (int, error) { ... rows, err := r.queryCNPGRows(ctx, ` - SELECT service_type, service_id + SELECT service_type, service_id, registration_source FROM ( ... + SELECT 'checker', checker_id, first_registered AS last_seen, status, registration_source + FROM checkers + WHERE status = 'pending' + AND first_seen IS NULL + AND first_registered < $6 ) AS stale_services`, cutoff, cutoff, cutoff, cutoff, cutoff, cutoff) if err != nil { return 0, fmt.Errorf("failed to query stale services: %w", err) } ... for rows.Next() { - var serviceType, serviceID string - if err := rows.Scan(&serviceType, &serviceID); err != nil { + var serviceType, serviceID, source string + if err := rows.Scan(&serviceType, &serviceID, &source); err != nil { r.logger.Error().Err(err).Msg("Failed to scan stale service") continue } - if err := r.DeleteService(ctx, serviceType, serviceID); err != nil { + // Directly purge without re-fetching, as we've already established it's purgable. + if err := r.purgeService(ctx, serviceType, serviceID, RegistrationSource(source)); err != nil { r.logger.Warn(). Err(err). Str("service_type", serviceType). Str("service_id", serviceID). Msg("Failed to purge service") continue } deletedCount++ } ... } +// purgeService is a helper for bulk deletion that skips redundant status checks. +func (r ServiceRegistry) purgeService(ctx context.Context, serviceType, serviceID string, source RegistrationSource) error { + // Emit deletion event BEFORE deleting + metadata := map[string]string{"service_id": serviceID} + if err := r.emitRegistrationEvent(ctx, "deleted", serviceType, serviceID, "", source, "system", metadata); err != nil { + // Log but don't fail the deletion + r.logger.Warn().Err(err).Msg("Failed to emit deletion event during purge") + } + + if err := r.deleteServiceCNPG(ctx, serviceType, serviceID); err != nil { + return err + } + + if serviceType == serviceTypePoller { + r.invalidatePollerCache() + } + return nil +} + `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a significant N+1 query performance issue in the `PurgeInactive` function and proposes an effective solution by introducing a dedicated helper to bypass redundant database lookups.	Medium
Possible issue	Improve error handling in service deletion Refactor `DeleteService` to handle errors from `Get...` calls immediately within each `case` block, rather than using a single deferred error check, to make the control flow more robust. pkg/registry/service_registry.go [641-677] func (r ServiceRegistry) DeleteService(ctx context.Context, serviceType, serviceID string) error { // Verify service is not active or pending var ( status ServiceStatus source RegistrationSource - err error ) switch serviceType { case serviceTypePoller: - var poller RegisteredPoller - poller, err = r.GetPoller(ctx, serviceID) - if err == nil && poller != nil { - status = poller.Status - source = poller.RegistrationSource + poller, err := r.GetPoller(ctx, serviceID) + if err != nil { + return fmt.Errorf("failed to get poller for deletion: %w", err) } + status = poller.Status + source = poller.RegistrationSource case serviceTypeAgent: - var agent RegisteredAgent - agent, err = r.GetAgent(ctx, serviceID) - if err == nil && agent != nil { - status = agent.Status - source = agent.RegistrationSource + agent, err := r.GetAgent(ctx, serviceID) + if err != nil { + return fmt.Errorf("failed to get agent for deletion: %w", err) } + status = agent.Status + source = agent.RegistrationSource case serviceTypeChecker: - var checker RegisteredChecker - checker, err = r.GetChecker(ctx, serviceID) - if err == nil && checker != nil { - status = checker.Status - source = checker.RegistrationSource + checker, err := r.GetChecker(ctx, serviceID) + if err != nil { + return fmt.Errorf("failed to get checker for deletion: %w", err) } + status = checker.Status + source = checker.RegistrationSource default: return fmt.Errorf("%w: %s", ErrUnknownServiceType, serviceType) } - - if err != nil { - return fmt.Errorf("service not found: %w", err) - } ... `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential logic flaw in error handling and proposes a more robust, fail-fast approach that improves code clarity and prevents subtle bugs.	Medium
Possible issue	Continue batch processing on single-item error In `InsertServiceRegistrationEvents`, handle errors for individual events by logging them and continuing the batch process, rather than returning immediately and failing the entire operation. pkg/db/cnpg_registry.go [209-216] `for _, event := range events { args, err := buildCNPGServiceRegistrationEventArgs(event) if err != nil { - return err + db.logger.Warn(). + Err(err). + Str("service_id", event.ServiceID). + Str("event_type", event.EventType). + Msg("skipping CNPG service registration event insert") + continue } batch.Queue(insertServiceRegistrationEventSQL, args...) queued++ }` Apply / Chat Suggestion importance[1-10]: 6 __ Why: The suggestion correctly identifies that a single failing event will abort the entire batch insertion and proposes a more robust error handling strategy that is consistent with other functions in the file.	Low
More

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1946#issuecomment-3538945649 Original created: 2025-11-16T16:56:06Z --- ## 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>General</td> <td> <details><summary>Avoid N+1 queries during service purge</summary> ___ **Optimize <code>PurgeInactive</code> to avoid N+1 queries by creating a private helper function that performs the deletion directly, bypassing the redundant status checks in <code>DeleteService</code>.** [pkg/registry/service_registry.go [712-805]](https://github.com/carverauto/serviceradar/pull/1946/files#diff-d6d6d09a58edde934a1d3f571ff5ce02f0475c5e85ecdec27888892aff8d9d1dR712-R805) ```diff func (r *ServiceRegistry) PurgeInactive(ctx context.Context, retentionPeriod time.Duration) (int, error) { ... rows, err := r.queryCNPGRows(ctx, ` - SELECT service_type, service_id + SELECT service_type, service_id, registration_source FROM ( ... + SELECT 'checker', checker_id, first_registered AS last_seen, status, registration_source + FROM checkers + WHERE status = 'pending' + AND first_seen IS NULL + AND first_registered < $6 ) AS stale_services`, cutoff, cutoff, cutoff, cutoff, cutoff, cutoff) if err != nil { return 0, fmt.Errorf("failed to query stale services: %w", err) } ... for rows.Next() { - var serviceType, serviceID string - if err := rows.Scan(&serviceType, &serviceID); err != nil { + var serviceType, serviceID, source string + if err := rows.Scan(&serviceType, &serviceID, &source); err != nil { r.logger.Error().Err(err).Msg("Failed to scan stale service") continue } - if err := r.DeleteService(ctx, serviceType, serviceID); err != nil { + // Directly purge without re-fetching, as we've already established it's purgable. + if err := r.purgeService(ctx, serviceType, serviceID, RegistrationSource(source)); err != nil { r.logger.Warn(). Err(err). Str("service_type", serviceType). Str("service_id", serviceID). Msg("Failed to purge service") continue } deletedCount++ } ... } +// purgeService is a helper for bulk deletion that skips redundant status checks. +func (r *ServiceRegistry) purgeService(ctx context.Context, serviceType, serviceID string, source RegistrationSource) error { + // Emit deletion event BEFORE deleting + metadata := map[string]string{"service_id": serviceID} + if err := r.emitRegistrationEvent(ctx, "deleted", serviceType, serviceID, "", source, "system", metadata); err != nil { + // Log but don't fail the deletion + r.logger.Warn().Err(err).Msg("Failed to emit deletion event during purge") + } + + if err := r.deleteServiceCNPG(ctx, serviceType, serviceID); err != nil { + return err + } + + if serviceType == serviceTypePoller { + r.invalidatePollerCache() + } + return nil +} + ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies a significant N+1 query performance issue in the `PurgeInactive` function and proposes an effective solution by introducing a dedicated helper to bypass redundant database lookups. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=2>Possible issue</td> <td> <details><summary>Improve error handling in service deletion</summary> ___ **Refactor <code>DeleteService</code> to handle errors from <code>Get...</code> calls immediately within each <code>case</code> block, rather than using a single deferred error check, to make the control flow more robust.** [pkg/registry/service_registry.go [641-677]](https://github.com/carverauto/serviceradar/pull/1946/files#diff-d6d6d09a58edde934a1d3f571ff5ce02f0475c5e85ecdec27888892aff8d9d1dR641-R677) ```diff func (r *ServiceRegistry) DeleteService(ctx context.Context, serviceType, serviceID string) error { // Verify service is not active or pending var ( status ServiceStatus source RegistrationSource - err error ) switch serviceType { case serviceTypePoller: - var poller *RegisteredPoller - poller, err = r.GetPoller(ctx, serviceID) - if err == nil && poller != nil { - status = poller.Status - source = poller.RegistrationSource + poller, err := r.GetPoller(ctx, serviceID) + if err != nil { + return fmt.Errorf("failed to get poller for deletion: %w", err) } + status = poller.Status + source = poller.RegistrationSource case serviceTypeAgent: - var agent *RegisteredAgent - agent, err = r.GetAgent(ctx, serviceID) - if err == nil && agent != nil { - status = agent.Status - source = agent.RegistrationSource + agent, err := r.GetAgent(ctx, serviceID) + if err != nil { + return fmt.Errorf("failed to get agent for deletion: %w", err) } + status = agent.Status + source = agent.RegistrationSource case serviceTypeChecker: - var checker *RegisteredChecker - checker, err = r.GetChecker(ctx, serviceID) - if err == nil && checker != nil { - status = checker.Status - source = checker.RegistrationSource + checker, err := r.GetChecker(ctx, serviceID) + if err != nil { + return fmt.Errorf("failed to get checker for deletion: %w", err) } + status = checker.Status + source = checker.RegistrationSource default: return fmt.Errorf("%w: %s", ErrUnknownServiceType, serviceType) } - - if err != nil { - return fmt.Errorf("service not found: %w", err) - } ... ``` `[To ensure code accuracy, apply this suggestion manually]` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies a potential logic flaw in error handling and proposes a more robust, fail-fast approach that improves code clarity and prevents subtle bugs. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Continue batch processing on single-item error</summary> ___ **In <code>InsertServiceRegistrationEvents</code>, handle errors for individual events by logging them and continuing the batch process, rather than returning immediately and failing the entire operation.** [pkg/db/cnpg_registry.go [209-216]](https://github.com/carverauto/serviceradar/pull/1946/files#diff-e31b8b854d9ba774d2f3ed9899b1f5902462cd0e63990a2898dcaf9bc171d571R209-R216) ```diff for _, event := range events { args, err := buildCNPGServiceRegistrationEventArgs(event) if err != nil { - return err + db.logger.Warn(). + Err(err). + Str("service_id", event.ServiceID). + Str("event_type", event.EventType). + Msg("skipping CNPG service registration event insert") + continue } batch.Queue(insertServiceRegistrationEventSQL, args...) queued++ } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 6</summary> __ Why: The suggestion correctly identifies that a single failing event will abort the entire batch insertion and proposes a more robust error handling strategy that is consistent with other functions in the file. </details></details></td><td align=center>Low </td></tr> <tr><td align="center" colspan="2"> - [ ] More  </td><td></td></tr></tbody></table>