carverauto/serviceradar

Fork 0

fixing oban scheduler #2697

Merged

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

2026-01-18 09:54:14 +00:00

mfreeman451 commented

2026-01-18 09:53:30 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #2350
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/2350
Original created: 2026-01-18T09:53:30Z
Original updated: 2026-01-18T09:55:06Z
Original head: carverauto/serviceradar:2345-bug-sweep-groups---new-group
Original base: staging
Original merged: 2026-01-18T09:54:14Z 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, Enhancement

Description

Changed Oban unavailable log level from warning to debug
- Added note about sweep schedule reconciler handling
Fixed Oban peer configuration to use tuple format
Changed default Oban enabled state from false to true
Refactored job catalog to support multiple AshOban resources
- Added ServiceCheck, Alert, and OnboardingPackage resources
- Generalized trigger entry creation logic
Added Oban running status check for admin access control

Diagram Walkthrough

flowchart LR
  A["Oban Configuration"] -->|"peer tuple format"| B["Fixed Peer Setup"]
  C["Log Level Changes"] -->|"warning to debug"| D["Reduced Alert Noise"]
  E["Default Enabled"] -->|"false to true"| F["Oban Active by Default"]
  G["Job Catalog"] -->|"multi-resource support"| H["Enhanced Job Discovery"]
  I["Admin Access"] -->|"runtime check"| J["Oban Status Validation"]

File Walkthrough

Relevant files

Bug fix

schedule_sweep_monitor.ex `Reduced log severity for Oban unavailable conditions` elixir/serviceradar_core/lib/serviceradar/sweep_jobs/changes/schedule_sweep_monitor.ex Changed log level from warning to debug for Oban unavailable errors Added explanatory note about sweep schedule reconciler handling Applied changes to both sweep monitor and data cleanup scheduling	+12/-8
config.exs `Fixed Oban peer configuration format` web-ng/config/config.exs Updated Oban peer configuration from atom to tuple format Changed `Oban.Peers.Database` to `{Oban.Peers.Database, []}`	+1/-1

Configuration changes

runtime.exs

Enabled Oban by default and fixed peer config

web-ng/config/runtime.exs

Changed default Oban enabled state from false to true
Updated Oban peer configuration in runtime config to tuple format

+2/-2

Enhancement

job_catalog.ex `Refactored job catalog for multi-resource support` web-ng/lib/serviceradar_web_ng/jobs/job_catalog.ex Refactored to support multiple AshOban resources beyond PollingSchedule Added ServiceCheck, Alert, and OnboardingPackage resources Generalized trigger entry creation with helper functions Added resource_label, resource_id, and resource_description functions	+64/-22
user_auth.ex `Added Oban runtime status validation for admin access` web-ng/lib/serviceradar_web_ng_web/user_auth.ex Added runtime check for Oban running status before granting access Modified admin access logic to verify Oban is active on current node Added oban_running? helper function using Oban.Registry	+15/-0

Imported from GitHub pull request. Original GitHub pull request: #2350 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/2350 Original created: 2026-01-18T09:53:30Z Original updated: 2026-01-18T09:55:06Z Original head: carverauto/serviceradar:2345-bug-sweep-groups---new-group Original base: staging Original merged: 2026-01-18T09:54:14Z 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, Enhancement ___ ### **Description** - Changed Oban unavailable log level from warning to debug - Added note about sweep schedule reconciler handling - Fixed Oban peer configuration to use tuple format - Changed default Oban enabled state from false to true - Refactored job catalog to support multiple AshOban resources - Added ServiceCheck, Alert, and OnboardingPackage resources - Generalized trigger entry creation logic - Added Oban running status check for admin access control ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Oban Configuration"] -->|"peer tuple format"| B["Fixed Peer Setup"] C["Log Level Changes"] -->|"warning to debug"| D["Reduced Alert Noise"] E["Default Enabled"] -->|"false to true"| F["Oban Active by Default"] G["Job Catalog"] -->|"multi-resource support"| H["Enhanced Job Discovery"] I["Admin Access"] -->|"runtime check"| J["Oban Status Validation"] ``` <details><summary><h3>File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix</strong></td><td><table> <tr> <td> <details> <summary><strong>schedule_sweep_monitor.ex</strong><dd><code>Reduced log severity for Oban unavailable conditions</code>          </dd></summary> <hr> elixir/serviceradar_core/lib/serviceradar/sweep_jobs/changes/schedule_sweep_monitor.ex <ul><li>Changed log level from warning to debug for Oban unavailable errors<br> <li> Added explanatory note about sweep schedule reconciler handling<br> <li> Applied changes to both sweep monitor and data cleanup scheduling</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2350/files#diff-aea351916ba684aaab2941779c2750ebd099e2d30d0c929358f53aebc674b31f">+12/-8</a>    </td> </tr> <tr> <td> <details> <summary><strong>config.exs</strong><dd><code>Fixed Oban peer configuration format</code>                                          </dd></summary> <hr> web-ng/config/config.exs <ul><li>Updated Oban peer configuration from atom to tuple format<br> <li> Changed <code>Oban.Peers.Database</code> to <code>{Oban.Peers.Database, []}</code></ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2350/files#diff-da0b524f083d350207155c247526098fdd68866d64e7e4eae3d96fdae31bcfb6">+1/-1</a>      </td> </tr> </table></td></tr><tr><td><strong>Configuration changes</strong></td><td><table> <tr> <td> <details> <summary><strong>runtime.exs</strong><dd><code>Enabled Oban by default and fixed peer config</code>                        </dd></summary> <hr> web-ng/config/runtime.exs <ul><li>Changed default Oban enabled state from false to true<br> <li> Updated Oban peer configuration in runtime config to tuple format</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2350/files#diff-f1093c1a9d74ba3cd35f31e2c50dec60b4fb9c241fc5ff9f62bcbcc0559691ae">+2/-2</a>      </td> </tr> </table></td></tr><tr><td><strong>Enhancement</strong></td><td><table> <tr> <td> <details> <summary><strong>job_catalog.ex</strong><dd><code>Refactored job catalog for multi-resource support</code>                </dd></summary> <hr> web-ng/lib/serviceradar_web_ng/jobs/job_catalog.ex <ul><li>Refactored to support multiple AshOban resources beyond <br>PollingSchedule<br> <li> Added ServiceCheck, Alert, and OnboardingPackage resources<br> <li> Generalized trigger entry creation with helper functions<br> <li> Added resource_label, resource_id, and resource_description functions</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2350/files#diff-6bbf0790c2cbff1e6e9136cb8b9c20a07dd30e83e119937a0e0fe90418b5e258">+64/-22</a>  </td> </tr> <tr> <td> <details> <summary><strong>user_auth.ex</strong><dd><code>Added Oban runtime status validation for admin access</code>        </dd></summary> <hr> web-ng/lib/serviceradar_web_ng_web/user_auth.ex <ul><li>Added runtime check for Oban running status before granting access<br> <li> Modified admin access logic to verify Oban is active on current node<br> <li> Added oban_running? helper function using Oban.Registry</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/2350/files#diff-eed904963eb2089bfce4e634d5229889213b12d02489e07af53f1b1982a42d78">+15/-0</a>    </td> </tr> </table></td></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2026-01-18 09:54:02 +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/2350#issuecomment-3765118848
Original created: 2026-01-18T09:54:02Z

ⓘ Your approaching your monthly quota for Qodo. Upgrade your plan

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance

🟢

No security concerns identified

No security vulnerabilities detected by AI analysis. Human verification advised for critical code.

Ticket Compliance

🟢

🎫 #2345

🟢	Address the sweep monitor/data cleanup "Oban unavailable" warning being emitted (avoid or reduce the warning-level noise when Oban is unavailable).

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: 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: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
Silent exception handling: New code rescues all exceptions and returns a default value without logging or context,
which can hide production failures and impede debugging.

Referred Code

defp oban_running? do
  case Oban.Registry.whereis(Oban) do
    pid when is_pid(pid) -> true
    _ -> false
  end
rescue
  _ -> false
end

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 logging: The new admin-gating behavior for Oban UI access changes authorization outcomes but does
not add any audit log entries that could support reconstructing access decisions.

Referred Code

    oban_access?(scope) and oban_running?() ->
      conn

    oban_access?(scope) ->
      conn
      |> put_flash(:error, "Oban is not running on this node.")
      |> redirect(to: ~p"/admin/jobs")
      |> halt()

    true ->
      conn
      |> put_flash(:error, "You don't have permission to access this page.")
      |> redirect(to: ~p"/analytics")
      |> halt()
  end
end

defp maybe_store_return_to(%{method: "GET"} = conn) do
  put_session(conn, :user_return_to, current_path(conn))
end



 ... (clipped 17 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

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

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2350#issuecomment-3765118848 Original created: 2026-01-18T09:54:02Z --- <pre>ⓘ Your approaching your monthly quota for Qodo. <a href="https://www.qodo.ai/pricing">Upgrade your plan</a></pre> ## PR Compliance Guide 🔍  Below is a summary of compliance checks for this PR:<br> <table><tbody><tr><td colspan='2'><strong>Security Compliance</strong></td></tr> <tr><td>🟢</td><td><details><summary><strong>No security concerns identified</strong></summary> No security vulnerabilities detected by AI analysis. Human verification advised for critical code. </details></td></tr> <tr><td colspan='2'><strong>Ticket Compliance</strong></td></tr> <tr><td>🟢</td> <td> <details> <summary>🎫 <a href=https://github.com/carverauto/serviceradar/issues/2345>#2345</a></summary> <table width='100%'><tbody> <tr><td rowspan=1>🟢</td> <td>Address the sweep monitor/data cleanup "Oban unavailable" warning being emitted (avoid or <br>reduce the warning-level noise when Oban is unavailable).<br></td></tr> </tbody></table> </details> </td></tr> <tr><td colspan='2'><strong>Codebase Duplication Compliance</strong></td></tr> <tr><td>⚪</td><td><details><summary><strong>Codebase context is not defined </strong></summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'><strong>Custom Compliance</strong></td></tr> <tr><td rowspan=4>🟢</td><td> <details><summary><strong>Generic: Meaningful Naming and Self-Documenting Code</strong></summary><br> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code <br>self-documenting<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Error Handling</strong></summary><br> **Objective:** To prevent the leakage of sensitive system information through error messages while <br>providing sufficient detail for internal debugging.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Secure Logging Practices</strong></summary><br> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive <br>information like PII, PHI, or cardholder data.<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary><strong>Generic: Security-First Input Validation and Data Handling</strong></summary><br> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent <br>vulnerabilities<br> **Status:** Passed<br> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=1>🔴</td> <td><details> <summary><strong>Generic: Robust Error Handling and Edge Case Management</strong></summary><br> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful <br>degradation<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2350/files#diff-eed904963eb2089bfce4e634d5229889213b12d02489e07af53f1b1982a42d78R228-R235'><strong>Silent exception handling</strong></a>: New code rescues all exceptions and returns a default value without logging or context, <br>which can hide production failures and impede debugging.<br> <details open><summary>Referred Code</summary> ```elixir defp oban_running? do case Oban.Registry.whereis(Oban) do pid when is_pid(pid) -> true _ -> false end rescue _ -> false 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 rowspan=1>⚪</td> <td><details> <summary><strong>Generic: Comprehensive Audit Trails</strong></summary><br> **Objective:** To create a detailed and reliable record of critical system actions for security analysis <br>and compliance.<br> **Status:** <br><a href='https://github.com/carverauto/serviceradar/pull/2350/files#diff-eed904963eb2089bfce4e634d5229889213b12d02489e07af53f1b1982a42d78R198-R235'><strong>Missing audit logging</strong></a>: The new admin-gating behavior for Oban UI access changes authorization outcomes but does <br>not add any audit log entries that could support reconstructing access decisions.<br> <details open><summary>Referred Code</summary> ```elixir oban_access?(scope) and oban_running?() -> conn oban_access?(scope) -> conn |> put_flash(:error, "Oban is not running on this node.") |> redirect(to: ~p"/admin/jobs") |> halt() true -> conn |> put_flash(:error, "You don't have permission to access this page.") |> redirect(to: ~p"/analytics") |> halt() end end defp maybe_store_return_to(%{method: "GET"} = conn) do put_session(conn, :user_return_to, current_path(conn)) end ... (clipped 17 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 align="center" colspan="2">   </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant<br> 🟡 - Partial Compliant<br> 🔴 - Not Compliant<br> ⚪ - Requires Further Human Verification<br> 🏷️ - Compliance label<br> </details>

qodo-code-review[bot] commented

2026-01-18 09:55: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/2350#issuecomment-3765119698
Original created: 2026-01-18T09:55:06Z

ⓘ Your approaching your monthly quota for Qodo. Upgrade your plan

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Clarify Oban's role in web-ng The PR enables Oban by default in `web-ng`, but a comment suggests `core-elx` handles all jobs. This creates an architectural ambiguity that should be clarified by updating the comment or documentation. Examples: web-ng/config/runtime.exs [181] `System.get_env("SERVICERADAR_WEB_NG_OBAN_ENABLED", "true") in ~w(true 1 yes)` web-ng/config/config.exs [103] `# No Cron plugin - core-elx handles all scheduled jobs` Solution Walkthrough: Before: `# web-ng/config/runtime.exs # Oban is disabled by default for the web-ng application oban_enabled = System.get_env("..._OBAN_ENABLED", "false") # web-ng/config/config.exs config :oban, plugins: [ # ... # Comment states core-elx handles all scheduled jobs # No Cron plugin - core-elx handles all scheduled jobs ] # web-ng/lib/serviceradar_web_ng/jobs/job_catalog.ex # Job catalog only considers PollingSchedule triggers def ash_oban_jobs do polling_schedule_triggers() end` After: # web-ng/config/runtime.exs # Oban is now enabled by default for the web-ng application oban_enabled = System.get_env("..._OBAN_ENABLED", "true") # web-ng/config/config.exs config :oban, plugins: [ # ... # This comment is now confusing given Oban is enabled by default # No Cron plugin - core-elx handles all scheduled jobs ] # web-ng/lib/serviceradar_web_ng/jobs/job_catalog.ex # Job catalog now handles triggers from multiple resources def ash_oban_jobs do ash_oban_resources() \|> Enum.flat_map(&resource_triggers/1) end Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a significant architectural ambiguity, as the code enables Oban in `web-ng` while a comment claims `core-elx` handles all scheduled jobs, impacting future maintenance and clarity.	Medium
General	Log errors instead of silently rescuing In the `resource_triggers/1` function, log any exceptions caught in the `rescue` block instead of silently returning an empty list to improve error visibility. web-ng/lib/serviceradar_web_ng/jobs/job_catalog.ex [180-190] `defp resource_triggers(resource) do case AshOban.Info.oban_triggers(resource) do {:ok, triggers} -> Enum.map(triggers, &trigger_entry(resource, &1)) _ -> [] end rescue - _ -> [] + e -> + Logger.error( + "Failed to get AshOban triggers for resource #{inspect(resource)}. Error: #{inspect(e)}" + ) + + [] end` Apply / Chat Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that swallowing errors silently is bad practice and proposes adding logging, which significantly improves debuggability without breaking the function's resilience.	Medium
General	Normalize Oban env var case Normalize the `SERVICERADAR_WEB_NG_OBAN_ENABLED` environment variable to lowercase before checking its value to support case-insensitive inputs like "TRUE" or "Yes". web-ng/config/runtime.exs [181] `-System.get_env("SERVICERADAR_WEB_NG_OBAN_ENABLED", "true") in ~w(true 1 yes) +System.get_env("SERVICERADAR_WEB_NG_OBAN_ENABLED", "true") +\|> String.downcase() +\|> Kernel.in(~w(true 1 yes))` Apply / Chat Suggestion importance[1-10]: 5 __ Why: This is a good suggestion for making the environment variable check more robust by handling case-insensitivity, which is a common source of configuration errors.	Low
Possible issue	Check Oban PID liveness In the `oban_running?` function, add a `Process.alive?` check to ensure the process ID (PID) found for Oban corresponds to a live process. web-ng/lib/serviceradar_web_ng_web/user_auth.ex [229-232] `case Oban.Registry.whereis(Oban) do - pid when is_pid(pid) -> true + pid when is_pid(pid) and Process.alive?(pid) -> true _ -> false end` Apply / Chat Suggestion importance[1-10]: 7 __ Why: This suggestion correctly identifies a potential race condition where the Oban process could be dead but still in the registry, and provides a simple, effective fix using `Process.alive?`.	Medium
More

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/2350#issuecomment-3765119698 Original created: 2026-01-18T09:55:06Z --- <pre>ⓘ Your approaching your monthly quota for Qodo. <a href="https://www.qodo.ai/pricing">Upgrade your plan</a></pre> ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td><strong>Category</strong></td><td align=left><strong>Suggestion                                                                                                                                    </strong></td><td align=center><strong>Impact</strong></td></tr><tbody><tr><td rowspan=1>High-level</td> <td> <details><summary>Clarify Oban's role in web-ng</summary> ___ **The PR enables Oban by default in <code>web-ng</code>, but a comment suggests <code>core-elx</code> <br>handles all jobs. This creates an architectural ambiguity that should be <br>clarified by updating the comment or documentation.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2350/files#diff-f1093c1a9d74ba3cd35f31e2c50dec60b4fb9c241fc5ff9f62bcbcc0559691aeR181-R181">web-ng/config/runtime.exs [181]</a> </summary> ```elixir System.get_env("SERVICERADAR_WEB_NG_OBAN_ENABLED", "true") in ~w(true 1 yes) ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/2350/files#diff-da0b524f083d350207155c247526098fdd68866d64e7e4eae3d96fdae31bcfb6R103-R103">web-ng/config/config.exs [103]</a> </summary> ```elixir # No Cron plugin - core-elx handles all scheduled jobs ``` </details> ### Solution Walkthrough: #### Before: ```elixir # web-ng/config/runtime.exs # Oban is disabled by default for the web-ng application oban_enabled = System.get_env("..._OBAN_ENABLED", "false") # web-ng/config/config.exs config :oban, plugins: [ # ... # Comment states core-elx handles all scheduled jobs # No Cron plugin - core-elx handles all scheduled jobs ] # web-ng/lib/serviceradar_web_ng/jobs/job_catalog.ex # Job catalog only considers PollingSchedule triggers def ash_oban_jobs do polling_schedule_triggers() end ``` #### After: ```elixir # web-ng/config/runtime.exs # Oban is now enabled by default for the web-ng application oban_enabled = System.get_env("..._OBAN_ENABLED", "true") # web-ng/config/config.exs config :oban, plugins: [ # ... # This comment is now confusing given Oban is enabled by default # No Cron plugin - core-elx handles all scheduled jobs ] # web-ng/lib/serviceradar_web_ng/jobs/job_catalog.ex # Job catalog now handles triggers from multiple resources def ash_oban_jobs do ash_oban_resources() |> Enum.flat_map(&resource_triggers/1) end ``` <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies a significant architectural ambiguity, as the code enables Oban in `web-ng` while a comment claims `core-elx` handles all scheduled jobs, impacting future maintenance and clarity. </details></details></td><td align=center>Medium </td></tr><tr><td rowspan=2>General</td> <td> <details><summary>Log errors instead of silently rescuing</summary> ___ **In the <code>resource_triggers/1</code> function, log any exceptions caught in the <code>rescue</code> <br>block instead of silently returning an empty list to improve error visibility.** [web-ng/lib/serviceradar_web_ng/jobs/job_catalog.ex [180-190]](https://github.com/carverauto/serviceradar/pull/2350/files#diff-6bbf0790c2cbff1e6e9136cb8b9c20a07dd30e83e119937a0e0fe90418b5e258R180-R190) ```diff defp resource_triggers(resource) do case AshOban.Info.oban_triggers(resource) do {:ok, triggers} -> Enum.map(triggers, &trigger_entry(resource, &1)) _ -> [] end rescue - _ -> [] + e -> + Logger.error( + "Failed to get AshOban triggers for resource #{inspect(resource)}. Error: #{inspect(e)}" + ) + + [] end ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: The suggestion correctly identifies that swallowing errors silently is bad practice and proposes adding logging, which significantly improves debuggability without breaking the function's resilience. </details></details></td><td align=center>Medium </td></tr><tr><td> <details><summary>Normalize Oban env var case</summary> ___ **Normalize the <code>SERVICERADAR_WEB_NG_OBAN_ENABLED</code> environment variable to lowercase <br>before checking its value to support case-insensitive inputs like "TRUE" or <br>"Yes".** [web-ng/config/runtime.exs [181]](https://github.com/carverauto/serviceradar/pull/2350/files#diff-f1093c1a9d74ba3cd35f31e2c50dec60b4fb9c241fc5ff9f62bcbcc0559691aeR181-R181) ```diff -System.get_env("SERVICERADAR_WEB_NG_OBAN_ENABLED", "true") in ~w(true 1 yes) +System.get_env("SERVICERADAR_WEB_NG_OBAN_ENABLED", "true") +|> String.downcase() +|> Kernel.in(~w(true 1 yes)) ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 5</summary> __ Why: This is a good suggestion for making the environment variable check more robust by handling case-insensitivity, which is a common source of configuration errors. </details></details></td><td align=center>Low </td></tr><tr><td rowspan=1>Possible issue</td> <td> <details><summary>Check Oban PID liveness</summary> ___ **In the <code>oban_running?</code> function, add a <code>Process.alive?</code> check to ensure the process <br>ID (PID) found for Oban corresponds to a live process.** [web-ng/lib/serviceradar_web_ng_web/user_auth.ex [229-232]](https://github.com/carverauto/serviceradar/pull/2350/files#diff-eed904963eb2089bfce4e634d5229889213b12d02489e07af53f1b1982a42d78R229-R232) ```diff case Oban.Registry.whereis(Oban) do - pid when is_pid(pid) -> true + pid when is_pid(pid) and Process.alive?(pid) -> true _ -> false end ``` - [ ] **Apply / Chat**  <details><summary>Suggestion importance[1-10]: 7</summary> __ Why: This suggestion correctly identifies a potential race condition where the Oban process could be dead but still in the registry, and provides a simple, effective fix using `Process.alive?`. </details></details></td><td align=center>Medium </td></tr> <tr><td align="center" colspan="2"> - [ ] More  </td><td></td></tr></tbody></table>