carverauto/serviceradar

Fork 0

helm release fix, watchers and kv update #2553

Merged

mfreeman451 merged 4 commits from refs/pull/2553/head into main

2025-12-13 00:16:58 +00:00

mfreeman451 commented

2025-12-12 23:37:33 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2114
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2114
Original created: 2025-12-12T23:37:33Z
Original updated: 2025-12-13T00:31:27Z
Original head: carverauto/serviceradar:demo/kv_watchers_missing
Original base: main
Original merged: 2025-12-13T00:16:58Z 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

Add graceful shutdown support with signal handling
Implement CNPG password file watching and hot-reload capability
Fix watcher snapshot TTL handling in KV store
Refactor service processor initialization logic
Add Helm installation step to release workflow

Diagram Walkthrough

flowchart LR
  A["Signal Handler"] -->|"Graceful Shutdown"| B["Context Cancellation"]
  C["CNPG Password File"] -->|"Watch Every Minute"| D["Password Change Detection"]
  D -->|"Rebuild Pool"| E["Database Reload"]
  F["Watcher Snapshot"] -->|"Check Staleness"| G["Mark as Stopped"]
  H["Service Init"] -->|"Refactor"| I["buildProcessor Function"]

File Walkthrough

Relevant files

Enhancement

main.go

Add CNPG password watching and graceful shutdown

cmd/consumers/db-event-writer/main.go

Add signal handling for graceful shutdown using signal.NotifyContext
Implement watchCNPGPassword goroutine for monitoring password file
changes
Refactor applyCNPGPassword to extract password file reading logic
Add new readCNPGPasswordFile helper function for file operations
Integrate password watcher with service lifecycle

+94/-9

service.go

Refactor service initialization and add database reload

pkg/consumers/db-event-writer/service.go

Extract processor initialization logic into new buildProcessor
function
Add ReloadDatabase method for hot-reloading database connections
Refactor UpdateConfig to use buildProcessor helper
Implement proper cleanup and restart of service on database reload
Add nil checks and error handling in processor building

+52/-13

Bug fix

watchers_snapshot.go `Fix watcher snapshot TTL handling in KV store` pkg/config/watchers_snapshot.go Export `WatcherSnapshotTTL` constant (rename from `watcherSnapshotTTL`) Add documentation comment explaining TTL usage for admin APIs Change KV store Put operation to use TTL of 0 instead of per-entry TTL Rely on `UpdatedAt` timestamp for staleness detection instead of backend TTL	+6/-2
auth.go `Add watcher snapshot staleness validation` pkg/core/api/auth.go Add staleness check for watcher snapshots in `loadWatcherSnapshot` Mark snapshots as stopped if `UpdatedAt` exceeds `WatcherSnapshotTTL` Set default error message for stale snapshots Use exported `WatcherSnapshotTTL` constant for comparison	+7/-0

Configuration changes

release.yml

Add Helm installation to release workflow

.github/workflows/release.yml

Add Helm installation step using azure/setup-helm@v4 action
Specify Helm version v3.14.4 for consistency
Install Helm before publishing chart to OCI registry

+5/-0

Imported from GitHub pull request. Original GitHub pull request: #2114 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2114 Original created: 2025-12-12T23:37:33Z Original updated: 2025-12-13T00:31:27Z Original head: carverauto/serviceradar:demo/kv_watchers_missing Original base: main Original merged: 2025-12-13T00:16:58Z 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** - Add graceful shutdown support with signal handling - Implement CNPG password file watching and hot-reload capability - Fix watcher snapshot TTL handling in KV store - Refactor service processor initialization logic - Add Helm installation step to release workflow ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Signal Handler"] -->|"Graceful Shutdown"| B["Context Cancellation"] C["CNPG Password File"] -->|"Watch Every Minute"| D["Password Change Detection"] D -->|"Rebuild Pool"| E["Database Reload"] F["Watcher Snapshot"] -->|"Check Staleness"| G["Mark as Stopped"] H["Service Init"] -->|"Refactor"| I["buildProcessor Function"] ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td>Enhancement</td><td><table> <tr> <td> <details> <summary>main.go<dd><code>Add CNPG password watching and graceful shutdown</code>                  </dd></summary> <hr> cmd/consumers/db-event-writer/main.go <ul><li>Add signal handling for graceful shutdown using <code>signal.NotifyContext</code> <li> Implement <code>watchCNPGPassword</code> goroutine for monitoring password file changes <li> Refactor <code>applyCNPGPassword</code> to extract password file reading logic <li> Add new <code>readCNPGPasswordFile</code> helper function for file operations <li> Integrate password watcher with service lifecycle</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2114/files#diff-c9a73828b631e4618af51a47bc4c618d72ad1726fef3c3cbe12ab73b57b0eb63">+94/-9</a>    </td> </tr> <tr> <td> <details> <summary>service.go<dd><code>Refactor service initialization and add database reload</code>    </dd></summary> <hr> pkg/consumers/db-event-writer/service.go <ul><li>Extract processor initialization logic into new <code>buildProcessor</code> function <li> Add <code>ReloadDatabase</code> method for hot-reloading database connections <li> Refactor <code>UpdateConfig</code> to use <code>buildProcessor</code> helper <li> Implement proper cleanup and restart of service on database reload <li> Add nil checks and error handling in processor building</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2114/files#diff-9f9f48b11e7670c7ae374abc41327adf4617972b214f1c168e9da53d3cd7b609">+52/-13</a>  </td> </tr> </table></td></tr><tr><td>Bug fix</td><td><table> <tr> <td> <details> <summary>watchers_snapshot.go<dd><code>Fix watcher snapshot TTL handling in KV store</code>                        </dd></summary> <hr> pkg/config/watchers_snapshot.go <ul><li>Export <code>WatcherSnapshotTTL</code> constant (rename from <code>watcherSnapshotTTL</code>) <li> Add documentation comment explaining TTL usage for admin APIs <li> Change KV store Put operation to use TTL of 0 instead of per-entry TTL <li> Rely on <code>UpdatedAt</code> timestamp for staleness detection instead of backend TTL</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2114/files#diff-e6b688991669ff2e740b25b031841a14db1e2da72068981f3f6fd1fd0e2a236b">+6/-2</a>      </td> </tr> <tr> <td> <details> <summary>auth.go<dd><code>Add watcher snapshot staleness validation</code>                                </dd></summary> <hr> pkg/core/api/auth.go <ul><li>Add staleness check for watcher snapshots in <code>loadWatcherSnapshot</code> <li> Mark snapshots as stopped if <code>UpdatedAt</code> exceeds <code>WatcherSnapshotTTL</code> <li> Set default error message for stale snapshots <li> Use exported <code>WatcherSnapshotTTL</code> constant for comparison</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2114/files#diff-61eb09279471ca59a1b57a306f0ad05988a8cb39eca163b32bfb86c53f8a553e">+7/-0</a>      </td> </tr> </table></td></tr><tr><td>Configuration changes</td><td><table> <tr> <td> <details> <summary>release.yml<dd><code>Add Helm installation to release workflow</code>                                </dd></summary> <hr> .github/workflows/release.yml <ul><li>Add Helm installation step using <code>azure/setup-helm@v4</code> action <li> Specify Helm version v3.14.4 for consistency <li> Install Helm before publishing chart to OCI registry</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2114/files#diff-87db21a973eed4fef5f32b267aa60fcee5cbdf03c67fafdc2a9b553bb0b15f34">+5/-0</a>      </td> </tr> </table></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-12-12 23:38:05 +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/2114#issuecomment-3648517350
Original created: 2025-12-12T23:38:05Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Improper shutdown handling Description: The password watcher rebuilds the database connection using context.Background() instead of the cancellable ctx, which can bypass graceful shutdown and lead to a lingering connection or resource leak if an attacker triggers repeated password change events. main.go [238-246] Referred Code `newDB, err := db.New(context.Background(), dbConfig, dbLogger) if err != nil { if log != nil { log.Error().Err(err).Msg("Failed to rebuild CNPG pool after password change") } continue } if err := svc.ReloadDatabase(ctx, newDB); err != nil {`
	Resource leak risk Description: On CNPG password change, the old database handle is only closed when ReloadDatabase succeeds; if ReloadDatabase fails, the newly created handle is closed but the original connection remains with an invalid password, potentially causing prolonged failed retries and resource exhaustion. main.go [232-256] Referred Code `if log != nil { log.Warn().Msg("CNPG password changed; rebuilding CNPG pool") } cfg.CNPG.Password = pwd newDB, err := db.New(context.Background(), dbConfig, dbLogger) if err != nil { if log != nil { log.Error().Err(err).Msg("Failed to rebuild CNPG pool after password change") } continue } if err := svc.ReloadDatabase(ctx, newDB); err != nil { _ = newDB.Close() if log != nil { log.Error().Err(err).Msg("Failed to reload db-event-writer after password change") } continue } ... (clipped 4 lines)`
	Stale data persistence Description: Changing KV Put TTL to 0 makes snapshot entries persistent; without backend-enforced expiration, a misconfigured or compromised writer could leave stale records indefinitely, increasing the risk of stale data being served if readers fail to enforce staleness correctly elsewhere. watchers_snapshot.go [52-55] Referred Code `// Do not rely on per-entry TTL support in the KV backend. Instead, embed an // UpdatedAt timestamp and let readers treat stale snapshots as offline. return m.client.Put(ctx, key, payload, 0) }`
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: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing audit logs: New critical actions like CNPG password reload and service restart are performed without explicit audit-style logging that captures actor/context beyond generic messages. Referred Code `if err != nil { if log != nil { log.Error().Err(err).Msg("Failed to rebuild CNPG pool after password change") } continue } if err := svc.ReloadDatabase(ctx, newDB); err != nil { _ = newDB.Close() if log != nil { log.Error().Err(err).Msg("Failed to reload db-event-writer after password change") } continue } if log != nil { log.Info().Msg("Reloaded db-event-writer after CNPG password change") }` 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: Context misuse: Database rebuild on password change uses context.Background() instead of the request context, potentially bypassing cancellation and timeouts on shutdown. Referred Code `newDB, err := db.New(context.Background(), dbConfig, dbLogger) if err != nil { if log != nil { log.Error().Err(err).Msg("Failed to rebuild CNPG pool after password change") } continue }` 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: Secret handling: Password file contents are read and used without explicit permission checks or file mode validation, which may risk insecure secret handling. Referred Code `func readCNPGPasswordFile(path string) (string, error) { if path == "" { return "", ErrCNPGPasswordRequired } data, err := os.ReadFile(path) if err != nil { return "", fmt.Errorf("read CNPG password file: %w", err) } pwd := strings.TrimSpace(string(data)) if pwd == "" { return "", fmt.Errorf("%w: %s", ErrCNPGPasswordEmpty, path) } return pwd, nil }` Learn more about managing compliance generic rules or creating your own custom rules
Update

Compliance status legend

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

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2114#issuecomment-3648517350 Original created: 2025-12-12T23:38:05Z --- ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR: <table><tbody><tr><td colspan='2'>Security Compliance</td></tr> <tr><td rowspan=3>⚪</td> <td><details><summary>Improper shutdown handling </summary> Description: The password watcher rebuilds the database connection using context.Background() instead of the cancellable ctx, which can bypass graceful shutdown and lead to a lingering connection or resource leak if an attacker triggers repeated password change events. <a href='https://github.com/carverauto/serviceradar/pull/2114/files#diff-c9a73828b631e4618af51a47bc4c618d72ad1726fef3c3cbe12ab73b57b0eb63R238-R246'>main.go [238-246]</a> <details open><summary>Referred Code</summary> ```go newDB, err := db.New(context.Background(), dbConfig, dbLogger) if err != nil { if log != nil { log.Error().Err(err).Msg("Failed to rebuild CNPG pool after password change") } continue } if err := svc.ReloadDatabase(ctx, newDB); err != nil { ``` </details></details></td></tr> <tr><td><details><summary>Resource leak risk </summary> Description: On CNPG password change, the old database handle is only closed when ReloadDatabase succeeds; if ReloadDatabase fails, the newly created handle is closed but the original connection remains with an invalid password, potentially causing prolonged failed retries and resource exhaustion. <a href='https://github.com/carverauto/serviceradar/pull/2114/files#diff-c9a73828b631e4618af51a47bc4c618d72ad1726fef3c3cbe12ab73b57b0eb63R232-R256'>main.go [232-256]</a> <details open><summary>Referred Code</summary> ```go if log != nil { log.Warn().Msg("CNPG password changed; rebuilding CNPG pool") } cfg.CNPG.Password = pwd newDB, err := db.New(context.Background(), dbConfig, dbLogger) if err != nil { if log != nil { log.Error().Err(err).Msg("Failed to rebuild CNPG pool after password change") } continue } if err := svc.ReloadDatabase(ctx, newDB); err != nil { _ = newDB.Close() if log != nil { log.Error().Err(err).Msg("Failed to reload db-event-writer after password change") } continue } ... (clipped 4 lines) ``` </details></details></td></tr> <tr><td><details><summary>Stale data persistence </summary> Description: Changing KV Put TTL to 0 makes snapshot entries persistent; without backend-enforced expiration, a misconfigured or compromised writer could leave stale records indefinitely, increasing the risk of stale data being served if readers fail to enforce staleness correctly elsewhere. <a href='https://github.com/carverauto/serviceradar/pull/2114/files#diff-e6b688991669ff2e740b25b031841a14db1e2da72068981f3f6fd1fd0e2a236bR52-R55'>watchers_snapshot.go [52-55]</a> <details open><summary>Referred Code</summary> ```go // Do not rely on per-entry TTL support in the KV backend. Instead, embed an // UpdatedAt timestamp and let readers treat stale snapshots as offline. return m.client.Put(ctx, key, payload, 0) } ``` </details></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=3>🟢</td><td> <details><summary>Generic: Meaningful Naming and Self-Documenting Code</summary> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code self-documenting **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary>Generic: Secure Error Handling</summary> **Objective:** To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary>Generic: Secure Logging Practices</summary> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=3>⚪</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:** <a href='https://github.com/carverauto/serviceradar/pull/2114/files#diff-c9a73828b631e4618af51a47bc4c618d72ad1726fef3c3cbe12ab73b57b0eb63R239-R256'>Missing audit logs</a>: New critical actions like CNPG password reload and service restart are performed without explicit audit-style logging that captures actor/context beyond generic messages. <details open><summary>Referred Code</summary> ```go if err != nil { if log != nil { log.Error().Err(err).Msg("Failed to rebuild CNPG pool after password change") } continue } if err := svc.ReloadDatabase(ctx, newDB); err != nil { _ = newDB.Close() if log != nil { log.Error().Err(err).Msg("Failed to reload db-event-writer after password change") } continue } if log != nil { log.Info().Msg("Reloaded db-event-writer after CNPG password change") } ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary>Generic: Robust Error Handling and Edge Case Management</summary> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful degradation **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2114/files#diff-c9a73828b631e4618af51a47bc4c618d72ad1726fef3c3cbe12ab73b57b0eb63R238-R244'>Context misuse</a>: Database rebuild on password change uses context.Background() instead of the request context, potentially bypassing cancellation and timeouts on shutdown. <details open><summary>Referred Code</summary> ```go newDB, err := db.New(context.Background(), dbConfig, dbLogger) if err != nil { if log != nil { log.Error().Err(err).Msg("Failed to rebuild CNPG pool after password change") } continue } ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary>Generic: Security-First Input Validation and Data Handling</summary> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2114/files#diff-c9a73828b631e4618af51a47bc4c618d72ad1726fef3c3cbe12ab73b57b0eb63R177-R193'>Secret handling</a>: Password file contents are read and used without explicit permission checks or file mode validation, which may risk insecure secret handling. <details open><summary>Referred Code</summary> ```go func readCNPGPasswordFile(path string) (string, error) { if path == "" { return "", ErrCNPGPasswordRequired } data, err := os.ReadFile(path) if err != nil { return "", fmt.Errorf("read CNPG password file: %w", err) } pwd := strings.TrimSpace(string(data)) if pwd == "" { return "", fmt.Errorf("%w: %s", ErrCNPGPasswordEmpty, path) } return pwd, nil } ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant 🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label </details>

qodo-code-review[bot] commented

2025-12-12 23:39:05 +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/2114#issuecomment-3648519165
Original created: 2025-12-12T23:39:05Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Simplify password rotation via orchestration Remove the custom logic for hot-reloading database passwords. Instead, rely on an orchestration platform like Kubernetes to handle password rotation by automatically restarting the application pod when its secrets are updated. Examples: cmd/consumers/db-event-writer/main.go [195-259] `func watchCNPGPassword( ctx context.Context, cfg dbeventwriter.DBEventWriterConfig, dbConfig models.CoreServiceConfig, dbLogger logger.Logger, svc dbeventwriter.Service, log logger.Logger, ) { if ctx == nil \|\| cfg == nil \|\| cfg.CNPG == nil \|\| svc == nil { return ... (clipped 55 lines)` pkg/consumers/db-event-writer/service.go [152-179] `// ReloadDatabase rebuilds the CNPG connection and restarts the NATS consumer loop. // It is safe to call while the service is running. func (s Service) ReloadDatabase(ctx context.Context, dbService db.Service) error { if dbService == nil { return nil } if s.runCancel != nil { s.runCancel() } ... (clipped 18 lines)` Solution Walkthrough: Before: // in main.go func main() { // ... go watchCNPGPassword(ctx, &cfg, dbConfig, dbLogger, svc, serviceLogger) // ... } func watchCNPGPassword(...) { ticker := time.NewTicker(time.Minute) for range ticker.C { // read password from file if new_password != old_password { // create new DB connection newDB, err := db.New(...) // reload the service with the new DB connection svc.ReloadDatabase(ctx, newDB) } } } // in service.go func (s *Service) ReloadDatabase(ctx context.Context, dbService db.Service) error { s.runCancel() // Stop existing consumer s.wg.Wait() // Wait for it to stop s.resetConnection() // ... swap database and processor instances ... _ = oldDB.Close() return s.Start(ctx) // Restart consumer } After: // in main.go func main() { // ... // The watchCNPGPassword goroutine is removed. // Password is read once at startup. if err := applyCNPGPassword(&cfg); err != nil { log.Fatalf("Failed to apply CNPG password: %v", err) } // ... create dbService and start server ... } // The `watchCNPGPassword` function in main.go and the `ReloadDatabase` // method in service.go are removed entirely. // The application now relies on the orchestration layer (e.g., Kubernetes) // to restart the pod upon secret changes. The application code is // simplified to only handle initial configuration loading. Suggestion importance[1-10]: 9 __ Why: This is an excellent architectural suggestion that correctly identifies significant new complexity and proposes a simpler, more robust, industry-standard alternative that simplifies application state management.	High
Possible issue	Use cancellable context for database connection In the `watchCNPGPassword` function, replace `context.Background()` with the cancellable `ctx` when calling `db.New` to ensure the operation can be aborted during application shutdown. cmd/consumers/db-event-writer/main.go [236-244] `cfg.CNPG.Password = pwd - newDB, err := db.New(context.Background(), dbConfig, dbLogger) + newDB, err := db.New(ctx, dbConfig, dbLogger) if err != nil { if log != nil { log.Error().Err(err).Msg("Failed to rebuild CNPG pool after password change") } continue }` Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly identifies a bug where using `context.Background()` in a cancellable goroutine can prevent graceful shutdown. Propagating the `ctx` is the correct fix and improves service reliability.	Medium
Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2114#issuecomment-3648519165 Original created: 2025-12-12T23:39:05Z --- ## 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>High-level</td> <td> <details><summary>Simplify password rotation via orchestration</summary> ___ **Remove the custom logic for hot-reloading database passwords. Instead, rely on an orchestration platform like Kubernetes to handle password rotation by automatically restarting the application pod when its secrets are updated.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2114/files#diff-c9a73828b631e4618af51a47bc4c618d72ad1726fef3c3cbe12ab73b57b0eb63R195-R259">cmd/consumers/db-event-writer/main.go [195-259]</a> </summary> ```go func watchCNPGPassword( ctx context.Context, cfg *dbeventwriter.DBEventWriterConfig, dbConfig *models.CoreServiceConfig, dbLogger logger.Logger, svc *dbeventwriter.Service, log logger.Logger, ) { if ctx == nil || cfg == nil || cfg.CNPG == nil || svc == nil { return ... (clipped 55 lines) ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2114/files#diff-9f9f48b11e7670c7ae374abc41327adf4617972b214f1c168e9da53d3cd7b609R152-R179">pkg/consumers/db-event-writer/service.go [152-179]</a> </summary> ```go // ReloadDatabase rebuilds the CNPG connection and restarts the NATS consumer loop. // It is safe to call while the service is running. func (s *Service) ReloadDatabase(ctx context.Context, dbService db.Service) error { if dbService == nil { return nil } if s.runCancel != nil { s.runCancel() } ... (clipped 18 lines) ``` </details> ### Solution Walkthrough: #### Before: ```go // in main.go func main() { // ... go watchCNPGPassword(ctx, &cfg, dbConfig, dbLogger, svc, serviceLogger) // ... } func watchCNPGPassword(...) { ticker := time.NewTicker(time.Minute) for range ticker.C { // read password from file if new_password != old_password { // create new DB connection newDB, err := db.New(...) // reload the service with the new DB connection svc.ReloadDatabase(ctx, newDB) } } } // in service.go func (s *Service) ReloadDatabase(ctx context.Context, dbService db.Service) error { s.runCancel() // Stop existing consumer s.wg.Wait() // Wait for it to stop s.resetConnection() // ... swap database and processor instances ... _ = oldDB.Close() return s.Start(ctx) // Restart consumer } ``` #### After: ```go // in main.go func main() { // ... // The watchCNPGPassword goroutine is removed. // Password is read once at startup. if err := applyCNPGPassword(&cfg); err != nil { log.Fatalf("Failed to apply CNPG password: %v", err) } // ... create dbService and start server ... } // The `watchCNPGPassword` function in main.go and the `ReloadDatabase` // method in service.go are removed entirely. // The application now relies on the orchestration layer (e.g., Kubernetes) // to restart the pod upon secret changes. The application code is // simplified to only handle initial configuration loading. ``` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: This is an excellent architectural suggestion that correctly identifies significant new complexity and proposes a simpler, more robust, industry-standard alternative that simplifies application state management. </details></details></td><td align=center>High </td></tr><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Use cancellable context for database connection</summary> ___ **In the <code>watchCNPGPassword</code> function, replace <code>context.Background()</code> with the cancellable <code>ctx</code> when calling <code>db.New</code> to ensure the operation can be aborted during application shutdown.** [cmd/consumers/db-event-writer/main.go [236-244]](https://github.com/carverauto/serviceradar/pull/2114/files#diff-c9a73828b631e4618af51a47bc4c618d72ad1726fef3c3cbe12ab73b57b0eb63R236-R244) ```diff cfg.CNPG.Password = pwd - newDB, err := db.New(context.Background(), dbConfig, dbLogger) + newDB, err := db.New(ctx, dbConfig, dbLogger) if err != nil { if log != nil { log.Error().Err(err).Msg("Failed to rebuild CNPG pool after password change") } continue } ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 8</summary> __ Why: The suggestion correctly identifies a bug where using `context.Background()` in a cancellable goroutine can prevent graceful shutdown. Propagating the `ctx` is the correct fix and improves service reliability. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>