carverauto/serviceradar

Fork 0

fix zen rule crash #2657

Merged

mfreeman451 merged 6 commits from refs/pull/2657/head into staging

2026-01-12 22:11:59 +00:00

mfreeman451 commented

2026-01-12 19:29:39 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2274
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2274
Original created: 2026-01-12T19:29:39Z
Original updated: 2026-01-12T22:12:00Z
Original head: carverauto/serviceradar:bug/core-zen-rule-crash
Original base: staging
Original merged: 2026-01-12T22:11:59Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

I have signed the DCO?
The build completes without errors?
All tests are passing when running make test?

PR Type

Bug fix

Description

Added exception handling to prevent crashes in zen rule sync
Improved error handling for JSON encoding failures
Added logging for unexpected results and exceptions
Ensured sync operations always return :ok status

Diagram Walkthrough

flowchart LR
  A["sync_rule_with_logging"] --> B["sync_rule_with_actor"]
  B --> C{"Result"}
  C -->|ok| D["Return :ok"]
  C -->|error| E["Log warning + Return :ok"]
  C -->|unexpected| F["Log warning + Return :ok"]
  A --> G["rescue clause"]
  G --> H["Log error + Return :ok"]
  I["sync_rule_impl"] --> J{"JSON encode"}
  J -->|success| K["Continue with Client.put"]
  J -->|failure| L["Return error tuple"]

File Walkthrough

Relevant files

Bug fix

zen_rule_sync.ex

Add exception handling and JSON encoding error recovery

elixir/serviceradar_core/lib/serviceradar/observability/zen_rule_sync.ex

Added rescue clause to catch and log exceptions in
sync_rule_with_logging/2
Added handling for unexpected return values with warning logs
Changed Jason.encode! to Jason.encode with proper error handling in
sync_rule_impl/3
Added explicit error handling for Jason.EncodeError in the with clause
Ensured all code paths return :ok to prevent process crashes

+31/-3

Imported from GitHub pull request. Original GitHub pull request: #2274 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2274 Original created: 2026-01-12T19:29:39Z Original updated: 2026-01-12T22:12:00Z Original head: carverauto/serviceradar:bug/core-zen-rule-crash Original base: staging Original merged: 2026-01-12T22:11:59Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Bug fix ___ ### **Description** - Added exception handling to prevent crashes in zen rule sync - Improved error handling for JSON encoding failures - Added logging for unexpected results and exceptions - Ensured sync operations always return :ok status ___ ### Diagram Walkthrough ```mermaid flowchart LR A["sync_rule_with_logging"] --> B["sync_rule_with_actor"] B --> C{"Result"} C -->|ok| D["Return :ok"] C -->|error| E["Log warning + Return :ok"] C -->|unexpected| F["Log warning + Return :ok"] A --> G["rescue clause"] G --> H["Log error + Return :ok"] I["sync_rule_impl"] --> J{"JSON encode"} J -->|success| K["Continue with Client.put"] J -->|failure| L["Return error tuple"] ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td>Bug fix</td><td><table> <tr> <td> <details> <summary>zen_rule_sync.ex<dd><code>Add exception handling and JSON encoding error recovery</code>    </dd></summary> <hr> elixir/serviceradar_core/lib/serviceradar/observability/zen_rule_sync.ex <ul><li>Added rescue clause to catch and log exceptions in <code>sync_rule_with_logging/2</code> <li> Added handling for unexpected return values with warning logs <li> Changed <code>Jason.encode!</code> to <code>Jason.encode</code> with proper error handling in <code>sync_rule_impl/3</code> <li> Added explicit error handling for <code>Jason.EncodeError</code> in the with clause <li> Ensured all code paths return <code>:ok</code> to prevent process crashes</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2274/files#diff-d5f190f1d1692ffe3e1dba929363e6ff1ecbd70ddcb1f6f2240473466fe4ec8a">+31/-3</a>    </td> </tr> </table></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2026-01-12 19:30:10 +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/2274#issuecomment-3740160383
Original created: 2026-01-12T19:30:10Z

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Sensitive data in logs Description: The new logging paths use `inspect(unexpected)` and `Exception.format(..., STACKTRACE)`, which may leak sensitive data (e.g., payload contents, tokens, or internal state present in exception messages/stacktraces) into logs if failures occur during Zen rule sync. zen_rule_sync.ex [227-244] Referred Code `unexpected -> Logger.warning("Zen rule reconcile returned unexpected result", tenant_id: state.tenant_id, rule_id: rule.id, result: inspect(unexpected) ) :ok end rescue error -> Logger.error("Zen rule reconcile crashed", tenant_id: state.tenant_id, rule_id: rule.id, error: Exception.format(:error, error, __STACKTRACE__) ) :ok`
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: 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: 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
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Missing actor context: New log entries for zen rule reconciliation include `tenant_id` and `rule_id` but do not include an explicit user/actor identifier, which may be required to reconstruct who initiated the action. Referred Code `Logger.warning("Zen rule reconcile failed", tenant_id: state.tenant_id, rule_id: rule.id, reason: inspect(reason) ) :ok unexpected -> Logger.warning("Zen rule reconcile returned unexpected result", tenant_id: state.tenant_id, rule_id: rule.id, result: inspect(unexpected) ) :ok end rescue error -> Logger.error("Zen rule reconcile crashed", tenant_id: state.tenant_id, ... (clipped 3 lines)` 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: Potential detail leakage: The new error tuple includes `Exception.message(error)` for JSON encode failures, which may expose internal details to upstream callers depending on how this error is surfaced. Referred Code `with {:ok, payload} <- Jason.encode(rule.compiled_jdm), :ok <- Client.put(key, payload), {:ok, _value, revision} <- Client.get_with_revision(key), :ok <- update_kv_revision(rule, revision, tenant_schema, actor) do {:ok, revision} else {:error, %Jason.EncodeError{} = error} -> {:error, {:json_encode_failed, Exception.message(error)}} {:error, reason} -> {:error, reason} end` 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: Sensitive data in logs: Logging `Exception.format(..., STACKTRACE)` and `inspect(unexpected)` may capture and emit sensitive data from exceptions/return values into logs depending on what data is present in the stacktrace or unexpected term. Referred Code `unexpected -> Logger.warning("Zen rule reconcile returned unexpected result", tenant_id: state.tenant_id, rule_id: rule.id, result: inspect(unexpected) ) :ok end rescue error -> Logger.error("Zen rule reconcile crashed", tenant_id: state.tenant_id, rule_id: rule.id, error: Exception.format(:error, error, __STACKTRACE__) )` 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/2274#issuecomment-3740160383 Original created: 2026-01-12T19:30:10Z --- ## 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=1>⚪</td> <td><details><summary>Sensitive data in logs </summary> Description: The new logging paths use <code>inspect(unexpected)</code> and <code>Exception.format(..., __STACKTRACE__)</code>, which may leak sensitive data (e.g., payload contents, tokens, or internal state present in exception messages/stacktraces) into logs if failures occur during Zen rule sync. <a href='https://github.com/carverauto/serviceradar/pull/2274/files#diff-d5f190f1d1692ffe3e1dba929363e6ff1ecbd70ddcb1f6f2240473466fe4ec8aR227-R244'>zen_rule_sync.ex [227-244]</a> <details open><summary>Referred Code</summary> ```elixir unexpected -> Logger.warning("Zen rule reconcile returned unexpected result", tenant_id: state.tenant_id, rule_id: rule.id, result: inspect(unexpected) ) :ok end rescue error -> Logger.error("Zen rule reconcile crashed", tenant_id: state.tenant_id, rule_id: rule.id, error: Exception.format(:error, error, __STACKTRACE__) ) :ok ``` </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: 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: 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 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/2274/files#diff-d5f190f1d1692ffe3e1dba929363e6ff1ecbd70ddcb1f6f2240473466fe4ec8aR219-R242'>Missing actor context</a>: New log entries for zen rule reconciliation include <code>tenant_id</code> and <code>rule_id</code> but do not include an explicit user/actor identifier, which may be required to reconstruct who initiated the action. <details open><summary>Referred Code</summary> ```elixir Logger.warning("Zen rule reconcile failed", tenant_id: state.tenant_id, rule_id: rule.id, reason: inspect(reason) ) :ok unexpected -> Logger.warning("Zen rule reconcile returned unexpected result", tenant_id: state.tenant_id, rule_id: rule.id, result: inspect(unexpected) ) :ok end rescue error -> Logger.error("Zen rule reconcile crashed", tenant_id: state.tenant_id, ... (clipped 3 lines) ``` </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: Secure Error Handling</summary> **Objective:** To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. **Status:** <a href='https://github.com/carverauto/serviceradar/pull/2274/files#diff-d5f190f1d1692ffe3e1dba929363e6ff1ecbd70ddcb1f6f2240473466fe4ec8aR262-R273'>Potential detail leakage</a>: The new error tuple includes <code>Exception.message(error)</code> for JSON encode failures, which may expose internal details to upstream callers depending on how this error is surfaced. <details open><summary>Referred Code</summary> ```elixir with {:ok, payload} <- Jason.encode(rule.compiled_jdm), :ok <- Client.put(key, payload), {:ok, _value, revision} <- Client.get_with_revision(key), :ok <- update_kv_revision(rule, revision, tenant_schema, actor) do {:ok, revision} else {:error, %Jason.EncodeError{} = error} -> {:error, {:json_encode_failed, Exception.message(error)}} {:error, reason} -> {:error, reason} end ``` </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: 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:** <a href='https://github.com/carverauto/serviceradar/pull/2274/files#diff-d5f190f1d1692ffe3e1dba929363e6ff1ecbd70ddcb1f6f2240473466fe4ec8aR227-R242'>Sensitive data in logs</a>: Logging <code>Exception.format(..., __STACKTRACE__)</code> and <code>inspect(unexpected)</code> may capture and emit sensitive data from exceptions/return values into logs depending on what data is present in the stacktrace or unexpected term. <details open><summary>Referred Code</summary> ```elixir unexpected -> Logger.warning("Zen rule reconcile returned unexpected result", tenant_id: state.tenant_id, rule_id: rule.id, result: inspect(unexpected) ) :ok end rescue error -> Logger.error("Zen rule reconcile crashed", tenant_id: state.tenant_id, rule_id: rule.id, error: Exception.format(:error, error, __STACKTRACE__) ) ``` </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

2026-01-12 19:31:22 +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/2274#issuecomment-3740164488
Original created: 2026-01-12T19:31:22Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Error handling may hide systemic problems The current error handling always returns `:ok`, which can hide systemic issues. Implement a circuit breaker to detect and react to repeated failures, preventing silent, indefinite failure loops. Examples: elixir/serviceradar_core/lib/serviceradar/observability/zen_rule_sync.ex [213-245] `defp sync_rule_with_logging(rule, state) do case sync_rule_with_actor(rule, state) do {:ok, _} -> :ok {:error, reason} -> Logger.warning("Zen rule reconcile failed", tenant_id: state.tenant_id, rule_id: rule.id, reason: inspect(reason) ... (clipped 23 lines)` Solution Walkthrough: Before: `defp sync_rule_with_logging(rule, state) do case sync_rule_with_actor(rule, state) do {:ok, _} -> :ok {:error, reason} -> Logger.warning("Zen rule reconcile failed", ...) :ok // Always returns :ok unexpected -> Logger.warning("Zen rule reconcile returned unexpected result", ...) :ok // Always returns :ok end rescue error -> Logger.error("Zen rule reconcile crashed", ...) :ok // Always returns :ok end` After: // State would need to track failure counts, e.g., state.consecutive_failures defp sync_rule_with_logging(rule, state) do case sync_rule_with_actor(rule, state) do {:ok, _} -> // state = reset_failure_count(state) :ok _any_error -> // state = increment_failure_count(state) Logger.warning(...) if state.consecutive_failures > FAILURE_THRESHOLD do Logger.error("Circuit breaker tripped due to repeated failures!") // Propagate error to supervisor to handle systemic failure {:error, :too_many_failures} else :ok // Tolerate a small number of failures end end end Suggestion importance[1-10]: 9 __ Why: This is a critical design-level suggestion that correctly identifies a significant risk introduced by the PR—masking systemic issues by always returning `:ok`—and proposes a valid architectural pattern to mitigate it.	High
Possible issue	Avoid catching exit signals in rescue Modify the `rescue` block to only catch `Exception`s, not all throwables. This prevents interference with OTP supervision mechanisms by not catching exit signals. elixir/serviceradar_core/lib/serviceradar/observability/zen_rule_sync.ex [236-244] `rescue - error -> + e in Exception -> Logger.error("Zen rule reconcile crashed", tenant_id: state.tenant_id, rule_id: rule.id, - error: Exception.format(:error, error, __STACKTRACE__) + error: Exception.format(:error, e, __STACKTRACE__) ) :ok` Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly points out that a bare `rescue` can interfere with OTP supervision principles by catching exit signals. Specifying `rescue e in Exception` is a significant improvement for robustness and adherence to Elixir best practices.	Medium
Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2274#issuecomment-3740164488 Original created: 2026-01-12T19:31:22Z --- ## 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>Error handling may hide systemic problems</summary> ___ **The current error handling always returns <code>:ok</code>, which can hide systemic issues. Implement a circuit breaker to detect and react to repeated failures, preventing silent, indefinite failure loops.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2274/files#diff-d5f190f1d1692ffe3e1dba929363e6ff1ecbd70ddcb1f6f2240473466fe4ec8aR213-R245">elixir/serviceradar_core/lib/serviceradar/observability/zen_rule_sync.ex [213-245]</a> </summary> ```elixir defp sync_rule_with_logging(rule, state) do case sync_rule_with_actor(rule, state) do {:ok, _} -> :ok {:error, reason} -> Logger.warning("Zen rule reconcile failed", tenant_id: state.tenant_id, rule_id: rule.id, reason: inspect(reason) ... (clipped 23 lines) ``` </details> ### Solution Walkthrough: #### Before: ```elixir defp sync_rule_with_logging(rule, state) do case sync_rule_with_actor(rule, state) do {:ok, _} -> :ok {:error, reason} -> Logger.warning("Zen rule reconcile failed", ...) :ok // Always returns :ok unexpected -> Logger.warning("Zen rule reconcile returned unexpected result", ...) :ok // Always returns :ok end rescue error -> Logger.error("Zen rule reconcile crashed", ...) :ok // Always returns :ok end ``` #### After: ```elixir // State would need to track failure counts, e.g., state.consecutive_failures defp sync_rule_with_logging(rule, state) do case sync_rule_with_actor(rule, state) do {:ok, _} -> // state = reset_failure_count(state) :ok _any_error -> // state = increment_failure_count(state) Logger.warning(...) if state.consecutive_failures > FAILURE_THRESHOLD do Logger.error("Circuit breaker tripped due to repeated failures!") // Propagate error to supervisor to handle systemic failure {:error, :too_many_failures} else :ok // Tolerate a small number of failures end end end ``` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: This is a critical design-level suggestion that correctly identifies a significant risk introduced by the PR—masking systemic issues by always returning `:ok`—and proposes a valid architectural pattern to mitigate it. </details></details></td><td align=center>High </td></tr><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Avoid catching exit signals in rescue</summary> ___ **Modify the <code>rescue</code> block to only catch <code>Exception</code>s, not all throwables. This prevents interference with OTP supervision mechanisms by not catching exit signals.** [elixir/serviceradar_core/lib/serviceradar/observability/zen_rule_sync.ex [236-244]](https://github.com/carverauto/serviceradar/pull/2274/files#diff-d5f190f1d1692ffe3e1dba929363e6ff1ecbd70ddcb1f6f2240473466fe4ec8aR236-R244) ```diff rescue - error -> + e in Exception -> Logger.error("Zen rule reconcile crashed", tenant_id: state.tenant_id, rule_id: rule.id, - error: Exception.format(:error, error, __STACKTRACE__) + error: Exception.format(:error, e, __STACKTRACE__) ) :ok ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly points out that a bare `rescue` can interfere with OTP supervision principles by catching exit signals. Specifying `rescue e in Exception` is a significant improvement for robustness and adherence to Elixir best practices. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>