carverauto/serviceradar

Fork 0

fix: camera stream bug #3107

Merged

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

2026-04-01 00:31:07 +00:00

mfreeman451 commented

2026-04-01 00:30:44 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #3109
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/3109
Original created: 2026-04-01T00:30:44Z
Original updated: 2026-04-01T00:35:19Z
Original head: carverauto/serviceradar:bug/camera-streams-not-opening
Original base: staging
Original merged: 2026-04-01T00:31:07Z by @mfreeman451

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?

Imported from GitHub pull request. Original GitHub pull request: #3109 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/3109 Original created: 2026-04-01T00:30:44Z Original updated: 2026-04-01T00:35:19Z Original head: carverauto/serviceradar:bug/camera-streams-not-opening Original base: staging Original merged: 2026-04-01T00:31:07Z by @mfreeman451 --- ## 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?

qodo-code-review[bot] commented

2026-04-01 00:30:57 +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/3109#issuecomment-4166590444
Original created: 2026-04-01T00:30:57Z

Review Summary by Qodo

Fix camera stream opening with idempotent error recovery

🐞 Bug fix

Walkthroughs

Description

• Add error recovery for camera relay session opening transitions
• Wrap mark_opening call with exception handling and idempotency logic
• Reload session state when mark_opening fails to detect advanced status
• Add comprehensive test for idempotent mark_opening behavior

Diagram

flowchart LR
  A["mark_opening call"] --> B["mark_session_opening_safely wrapper"]
  B --> C{Exception or error?}
  C -->|Yes| D["recover_mark_opening_transition"]
  D --> E["Reload session state"]
  E --> F{Status opening/active?}
  F -->|Yes| G["Return recovered session"]
  F -->|No| H["Return error"]
  C -->|No| I["Return result"]

File Changes

1. elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex 🐞 Bug fix +41/-2

Add idempotent error recovery for mark_opening transitions

• Add Logger module requirement for warning messages
• Replace direct mark_opening call with mark_session_opening_safely wrapper function
• Implement error recovery logic that catches exceptions and validation errors
• Reload session state to detect if it already advanced to opening/active status
• Return recovered session if state transition succeeded despite error

elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex

2. elixir/serviceradar_core/test/serviceradar/camera/relay_session_manager_test.exs 🧪 Tests +59/-0

Test idempotent mark_opening error recovery
• Add new test case for idempotent mark_opening behavior
• Test scenario where mark_opening raises KeyError but session already advanced
• Verify session_loader is called to reload session state
• Assert successful return with opening status despite initial error
elixir/serviceradar_core/test/serviceradar/camera/relay_session_manager_test.exs

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/3109#issuecomment-4166590444 Original created: 2026-04-01T00:30:57Z --- <h3>Review Summary by Qodo</h3> Fix camera stream opening with idempotent error recovery <code>🐞 Bug fix</code> <img src="https://www.qodo.ai/wp-content/uploads/2025/11/light-grey-line.svg" height="10%" alt="Grey Divider"> <h3>Walkthroughs</h3> <details open> <summary>Description</summary> <pre> • Add error recovery for camera relay session opening transitions • Wrap mark_opening call with exception handling and idempotency logic • Reload session state when mark_opening fails to detect advanced status • Add comprehensive test for idempotent mark_opening behavior </pre> </details> <details> <summary>Diagram</summary> > ```mermaid flowchart LR A["mark_opening call"] --> B["mark_session_opening_safely wrapper"] B --> C{Exception or error?} C -->|Yes| D["recover_mark_opening_transition"] D --> E["Reload session state"] E --> F{Status opening/active?} F -->|Yes| G["Return recovered session"] F -->|No| H["Return error"] C -->|No| I["Return result"] ``` </details> <img src="https://www.qodo.ai/wp-content/uploads/2025/11/light-grey-line.svg" height="10%" alt="Grey Divider"> <h3>File Changes</h3> <details> <summary>1. elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex <code>🐞 Bug fix</code> <code> +41/-2 </code> </summary> >Add idempotent error recovery for mark_opening transitions ><pre> >• Add Logger module requirement for warning messages >• Replace direct mark_opening call with mark_session_opening_safely wrapper function >• Implement error recovery logic that catches exceptions and validation errors >• Reload session state to detect if it already advanced to opening/active status >• Return recovered session if state transition succeeded despite error ></pre> > ><a href='https://github.com/carverauto/serviceradar/pull/3109/files#diff-b33d053fadf147d3c148bd684d4273083ebdc56f3a6178b7d44cfe1461ebf5e0'> elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex </a> <hr/> </details> <details> <summary>2. elixir/serviceradar_core/test/serviceradar/camera/relay_session_manager_test.exs <code>🧪 Tests</code> <code> +59/-0 </code> </summary> >Test idempotent mark_opening error recovery ><pre> >• Add new test case for idempotent mark_opening behavior >• Test scenario where mark_opening raises KeyError but session already advanced >• Verify session_loader is called to reload session state >• Assert successful return with opening status despite initial error ></pre> > ><a href='https://github.com/carverauto/serviceradar/pull/3109/files#diff-1f85f794d268d938fca6b253f6ef587a91041908d095e436d24ebdfbae598cba'> elixir/serviceradar_core/test/serviceradar/camera/relay_session_manager_test.exs </a> <hr/> </details> <img src="https://www.qodo.ai/wp-content/uploads/2025/11/light-grey-line.svg" height="10%" alt="Grey Divider"> <a href="https://www.qodo.ai"><img src="https://www.qodo.ai/wp-content/uploads/2025/03/qodo-logo.svg" width="80" alt="Qodo Logo"></a>

qodo-code-review[bot] commented

2026-04-01 00:30:58 +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/3109#issuecomment-4166590527
Original created: 2026-04-01T00:30:58Z

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (1) 📎 Requirement gaps (0)

1. New test uses Ecto.UUID.generate() 📘 Rule violation ⚙ Maintainability

Description

The new relay session manager test generates IDs via Ecto.UUID.generate(), introducing direct Ecto
usage in new code where Ash UUID helpers exist. This conflicts with the guideline to prefer Ash
concepts over Ecto-centric patterns.

Code

elixir/serviceradar_core/test/serviceradar/camera/relay_session_manager_test.exs[R148-167]

+    camera_source_id = Ecto.UUID.generate()
+    stream_profile_id = Ecto.UUID.generate()
+    relay_session_id = Ecto.UUID.generate()
+
+    source_fetcher = fn ^camera_source_id ->
+      {:ok,
+       %{id: camera_source_id, assigned_agent_id: "agent-1", assigned_gateway_id: "gateway-1"}}
+    end
+
+    profile_fetcher = fn ^camera_source_id, ^stream_profile_id ->
+      {:ok, %{id: stream_profile_id, camera_source_id: camera_source_id}}
+    end
+
+    session_creator = fn attrs, _actor ->
+      {:ok, Map.put(attrs, :id, relay_session_id)}
+    end
+
+    dispatch_open = fn _agent_id, _payload, _opts, _actor ->
+      {:ok, Ecto.UUID.generate()}
+    end

Evidence
PR Compliance ID 5 requires avoiding new direct Ecto usage where Ash equivalents exist. The added
test lines call Ecto.UUID.generate() multiple times, while the codebase also uses
Ash.UUID.generate() as an alternative.
AGENTS.md
elixir/serviceradar_core/test/serviceradar/camera/relay_session_manager_test.exs[148-167]
elixir/serviceradar_core/lib/serviceradar/results_router.ex[262-264]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
New test code uses `Ecto.UUID.generate()` for UUID creation, which is direct Ecto usage.

## Issue Context
The compliance checklist requires preferring Ash concepts over Ecto-centric patterns when possible; the codebase already uses `Ash.UUID.generate()`.

## Fix Focus Areas
- elixir/serviceradar_core/test/serviceradar/camera/relay_session_manager_test.exs[148-167]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. Recovery logs warn too often 🐞 Bug ✧ Quality

Description

recover_mark_opening_transition/3 emits Logger.warning for every recovered mark_opening
failure when the session is already :opening/:active, which can create noisy warning logs during
expected race/idempotency scenarios. This can drown out actionable warnings and increase log volume
in production.

Code

elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex[R233-236]

+      {:ok, %{status: status} = session} when status in [:opening, :active] ->
+        Logger.warning(
+          "Recovered camera relay mark_opening transition for #{session_id} after #{inspect(reason)}"
+        )

Evidence
The new recovery path explicitly treats :opening and :active as successful recovery states, but
still logs at warning level when it does so.
elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex[230-242]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`recover_mark_opening_transition/3` logs a warning whenever it recovers by reloading a session in `:opening`/`:active`, even though this may be an expected race (the whole point of the new safe wrapper). This can flood warning logs.

### Issue Context
The recovery branch returns `{:ok, session}` when the session has already advanced; logging that at warning level can be too noisy.

### Fix Focus Areas
- elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex[230-242]

### Suggested fix
- Change the log to `Logger.debug/1` (or `Logger.info/1`) for the expected recovery case.
- Alternatively, only log a warning for unexpected reasons (e.g., when the reloaded session is not in a recoverable state), and keep the expected idempotent recovery quiet or debug-level.
- Consider adding structured metadata (e.g., `session_id:` and `reason:`) instead of interpolated strings.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. Active recovery drops open metadata 🐞 Bug ≡ Correctness

Description

The recovery path returns {:ok, session} for status in [:opening, :active], but :active can be
reached from :requested via RelaySession.activate/3 without ever persisting
command_id/lease_token (those are only accepted by mark_opening). As a result,
request_open/3 can succeed while returning a session missing open metadata that the manager just
generated (e.g., command_id/lease_token).

Code

elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex[R230-239]

+  defp recover_mark_opening_transition(%{id: session_id}, reason, session_loader)
+       when is_binary(session_id) do
+    case session_loader.(session_id) do
+      {:ok, %{status: status} = session} when status in [:opening, :active] ->
+        Logger.warning(
+          "Recovered camera relay mark_opening transition for #{session_id} after #{inspect(reason)}"
+        )
+
+        {:ok, session}
+

Evidence
recover_mark_opening_transition/3 treats :active as a successful recovery state. Separately, the
RelaySession state machine allows activation directly from :requested and the activate action
does not accept command_id/lease_token, while mark_opening is the action that accepts them, so
an :active session may legitimately not have those fields persisted.
elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex[230-242]
elixir/serviceradar_core/lib/serviceradar/camera/relay_session.ex[39-57]
elixir/serviceradar_core/lib/serviceradar/camera/relay_session.ex[73-85]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`recover_mark_opening_transition/3` returns success when the session is already `:active`, but `RelaySession.activate` can move a session from `:requested` to `:active` without setting `command_id`/`lease_token`. This means `request_open/3` can return an `:active` session that never recorded the open command metadata.

### Issue Context
- `RelaySession.activate` allows transition from `:requested` to `:active`.
- Only `mark_opening` accepts `command_id` and `lease_token`.
- The new recovery path accepts `:active` as success without ensuring those fields exist.

### Fix Focus Areas
- elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex[212-245]
- elixir/serviceradar_core/lib/serviceradar/camera/relay_session.ex[39-111]

### Suggested fix
Option A (best):
- Add a new RelaySession update action (system-only) that can set `command_id`, `lease_token`, and `lease_expires_at` **without** changing `status`.
- In `mark_session_opening_safely/7`, when recovery loads a session in `:active` and it is missing `command_id`/`lease_token`, call this new action to backfill metadata.

Option B (minimal):
- If you intentionally allow `:active` sessions without those fields, add an explicit log/telemetry event (debug/info) when recovery returns an `:active` session with missing metadata so the condition is detectable.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

ⓘ The new review experience is currently in Beta. Learn more

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/3109#issuecomment-4166590527 Original created: 2026-04-01T00:30:58Z --- <h3>Code Review by Qodo</h3> <code>🐞 Bugs (2)</code> <code>📘 Rule violations (1)</code> <code>📎 Requirement gaps (0)</code> <img src="https://www.qodo.ai/wp-content/uploads/2025/11/light-grey-line.svg" height="10%" alt="Grey Divider"> <img src="https://www.qodo.ai/wp-content/uploads/2026/01/review-recommended.png" height="20" alt="Remediation recommended"> <details> <summary> 1. New test uses Ecto.UUID.generate() <code>📘 Rule violation</code> <code>⚙ Maintainability</code></summary> > <details open> ><summary>Description</summary> > > ><pre> >The new relay session manager test generates IDs via Ecto.UUID.generate(), introducing direct Ecto >usage in new code where Ash UUID helpers exist. This conflicts with the guideline to prefer Ash >concepts over Ecto-centric patterns. ></pre> ></details> > <details open> ><summary>Code</summary> > > ><code>[elixir/serviceradar_core/test/serviceradar/camera/relay_session_manager_test.exs[R148-167]](https://github.com/carverauto/serviceradar/pull/3109/files#diff-1f85f794d268d938fca6b253f6ef587a91041908d095e436d24ebdfbae598cbaR148-R167)</code> > >```diff >+ camera_source_id = Ecto.UUID.generate() >+ stream_profile_id = Ecto.UUID.generate() >+ relay_session_id = Ecto.UUID.generate() >+ >+ source_fetcher = fn ^camera_source_id -> >+ {:ok, >+ %{id: camera_source_id, assigned_agent_id: "agent-1", assigned_gateway_id: "gateway-1"}} >+ end >+ >+ profile_fetcher = fn ^camera_source_id, ^stream_profile_id -> >+ {:ok, %{id: stream_profile_id, camera_source_id: camera_source_id}} >+ end >+ >+ session_creator = fn attrs, _actor -> >+ {:ok, Map.put(attrs, :id, relay_session_id)} >+ end >+ >+ dispatch_open = fn _agent_id, _payload, _opts, _actor -> >+ {:ok, Ecto.UUID.generate()} >+ end >``` ></details> > <details> ><summary>Evidence</summary> > > ><pre> >PR Compliance ID 5 requires avoiding new direct Ecto usage where Ash equivalents exist. The added >test lines call Ecto.UUID.generate() multiple times, while the codebase also uses >Ash.UUID.generate() as an alternative. ></pre> > > <code>AGENTS.md</code> > <code>[elixir/serviceradar_core/test/serviceradar/camera/relay_session_manager_test.exs[148-167]](https://github.com/carverauto/serviceradar/blob/32a2b4ae279f97a85aa318df562f3e3af94888f6/elixir/serviceradar_core/test/serviceradar/camera/relay_session_manager_test.exs/#L148-L167)</code> > <code>[elixir/serviceradar_core/lib/serviceradar/results_router.ex[262-264]](https://github.com/carverauto/serviceradar/blob/32a2b4ae279f97a85aa318df562f3e3af94888f6/elixir/serviceradar_core/lib/serviceradar/results_router.ex/#L262-L264)</code> ></details> > <details> ><summary>Agent prompt</summary> > > >``` >The issue below was found during a code review. Follow the provided context and guidance below and implement a solution > >## Issue description >New test code uses `Ecto.UUID.generate()` for UUID creation, which is direct Ecto usage. > >## Issue Context >The compliance checklist requires preferring Ash concepts over Ecto-centric patterns when possible; the codebase already uses `Ash.UUID.generate()`. > >## Fix Focus Areas >- elixir/serviceradar_core/test/serviceradar/camera/relay_session_manager_test.exs[148-167] >``` > <code>ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools</code> ></details> <hr/> </details> <details> <summary> 2. Recovery logs warn too often <code>🐞 Bug</code> <code>✧ Quality</code></summary> > <details open> ><summary>Description</summary> > > ><pre> >recover_mark_opening_transition/3 emits Logger.warning for every recovered mark_opening >failure when the session is already :opening/:active, which can create noisy warning logs during >expected race/idempotency scenarios. This can drown out actionable warnings and increase log volume >in production. ></pre> ></details> > <details open> ><summary>Code</summary> > > ><code>[elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex[R233-236]](https://github.com/carverauto/serviceradar/pull/3109/files#diff-b33d053fadf147d3c148bd684d4273083ebdc56f3a6178b7d44cfe1461ebf5e0R233-R236)</code> > >```diff >+ {:ok, %{status: status} = session} when status in [:opening, :active] -> >+ Logger.warning( >+ "Recovered camera relay mark_opening transition for #{session_id} after #{inspect(reason)}" >+ ) >``` ></details> > <details> ><summary>Evidence</summary> > > ><pre> >The new recovery path explicitly treats :opening and :active as successful recovery states, but >still logs at warning level when it does so. ></pre> > > <code>[elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex[230-242]](https://github.com/carverauto/serviceradar/blob/32a2b4ae279f97a85aa318df562f3e3af94888f6/elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex/#L230-L242)</code> ></details> > <details> ><summary>Agent prompt</summary> > > >``` >The issue below was found during a code review. Follow the provided context and guidance below and implement a solution > >### Issue description >`recover_mark_opening_transition/3` logs a warning whenever it recovers by reloading a session in `:opening`/`:active`, even though this may be an expected race (the whole point of the new safe wrapper). This can flood warning logs. > >### Issue Context >The recovery branch returns `{:ok, session}` when the session has already advanced; logging that at warning level can be too noisy. > >### Fix Focus Areas >- elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex[230-242] > >### Suggested fix >- Change the log to `Logger.debug/1` (or `Logger.info/1`) for the expected recovery case. >- Alternatively, only log a warning for unexpected reasons (e.g., when the reloaded session is not in a recoverable state), and keep the expected idempotent recovery quiet or debug-level. >- Consider adding structured metadata (e.g., `session_id:` and `reason:`) instead of interpolated strings. >``` > <code>ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools</code> ></details> <hr/> </details> <details> <summary> 3. Active recovery drops open metadata <code>🐞 Bug</code> <code>≡ Correctness</code></summary> > <details open> ><summary>Description</summary> > > ><pre> >The recovery path returns {:ok, session} for status in [:opening, :active], but :active can be >reached from :requested via RelaySession.activate/3 without ever persisting >command_id/lease_token (those are only accepted by mark_opening). As a result, >request_open/3 can succeed while returning a session missing open metadata that the manager just >generated (e.g., command_id/lease_token). ></pre> ></details> > <details open> ><summary>Code</summary> > > ><code>[elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex[R230-239]](https://github.com/carverauto/serviceradar/pull/3109/files#diff-b33d053fadf147d3c148bd684d4273083ebdc56f3a6178b7d44cfe1461ebf5e0R230-R239)</code> > >```diff >+ defp recover_mark_opening_transition(%{id: session_id}, reason, session_loader) >+ when is_binary(session_id) do >+ case session_loader.(session_id) do >+ {:ok, %{status: status} = session} when status in [:opening, :active] -> >+ Logger.warning( >+ "Recovered camera relay mark_opening transition for #{session_id} after #{inspect(reason)}" >+ ) >+ >+ {:ok, session} >+ >``` ></details> > <details> ><summary>Evidence</summary> > > ><pre> >recover_mark_opening_transition/3 treats :active as a successful recovery state. Separately, the >RelaySession state machine allows activation directly from :requested and the activate action >does not accept command_id/lease_token, while mark_opening is the action that accepts them, so >an :active session may legitimately not have those fields persisted. ></pre> > > <code>[elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex[230-242]](https://github.com/carverauto/serviceradar/blob/32a2b4ae279f97a85aa318df562f3e3af94888f6/elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex/#L230-L242)</code> > <code>[elixir/serviceradar_core/lib/serviceradar/camera/relay_session.ex[39-57]](https://github.com/carverauto/serviceradar/blob/32a2b4ae279f97a85aa318df562f3e3af94888f6/elixir/serviceradar_core/lib/serviceradar/camera/relay_session.ex/#L39-L57)</code> > <code>[elixir/serviceradar_core/lib/serviceradar/camera/relay_session.ex[73-85]](https://github.com/carverauto/serviceradar/blob/32a2b4ae279f97a85aa318df562f3e3af94888f6/elixir/serviceradar_core/lib/serviceradar/camera/relay_session.ex/#L73-L85)</code> ></details> > <details> ><summary>Agent prompt</summary> > > >``` >The issue below was found during a code review. Follow the provided context and guidance below and implement a solution > >### Issue description >`recover_mark_opening_transition/3` returns success when the session is already `:active`, but `RelaySession.activate` can move a session from `:requested` to `:active` without setting `command_id`/`lease_token`. This means `request_open/3` can return an `:active` session that never recorded the open command metadata. > >### Issue Context >- `RelaySession.activate` allows transition from `:requested` to `:active`. >- Only `mark_opening` accepts `command_id` and `lease_token`. >- The new recovery path accepts `:active` as success without ensuring those fields exist. > >### Fix Focus Areas >- elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex[212-245] >- elixir/serviceradar_core/lib/serviceradar/camera/relay_session.ex[39-111] > >### Suggested fix >Option A (best): >- Add a new RelaySession update action (system-only) that can set `command_id`, `lease_token`, and `lease_expires_at` **without** changing `status`. >- In `mark_session_opening_safely/7`, when recovery loads a session in `:active` and it is missing `command_id`/`lease_token`, call this new action to backfill metadata. > >Option B (minimal): >- If you intentionally allow `:active` sessions without those fields, add an explicit log/telemetry event (debug/info) when recovery returns an `:active` session with missing metadata so the condition is detectable. >``` > <code>ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools</code> ></details> <hr/> </details> <img src="https://www.qodo.ai/wp-content/uploads/2025/11/light-grey-line.svg" height="10%" alt="Grey Divider"> <pre>ⓘ The new review experience is currently in Beta. <a href="https://docs.qodo.ai/qodo-documentation/code-review">Learn more</a></pre> <img src="https://www.qodo.ai/wp-content/uploads/2025/11/light-grey-line.svg" height="10%" alt="Grey Divider">  <a href="https://www.qodo.ai"><img src="https://www.qodo.ai/wp-content/uploads/2025/03/qodo-logo.svg" width="80" alt="Qodo Logo"></a>

Copilot commented

2026-04-01 00:35:04 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR review comment.

Original author: @Copilot
Original URL: https://github.com/carverauto/serviceradar/pull/3109#discussion_r3019139696
Original created: 2026-04-01T00:35:04Z
Original path: elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex
Original line: 228

mark_session_opening_safely/7 rescues all exceptions and then attempts to treat them as an idempotent state transition. This can hide unexpected programming/runtime errors (e.g., KeyError/FunctionClauseError) and potentially return {:ok, session} even when mark_opening crashed for an unrelated reason. Consider rescuing only the specific Ash/state-machine transition errors that indicate the session already moved past :requested, and re-raising (or returning {:error, error} without recovery) for anything else.

Imported GitHub PR review comment. Original author: @Copilot Original URL: https://github.com/carverauto/serviceradar/pull/3109#discussion_r3019139696 Original created: 2026-04-01T00:35:04Z Original path: elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex Original line: 228 --- `mark_session_opening_safely/7` rescues all exceptions and then attempts to treat them as an idempotent state transition. This can hide unexpected programming/runtime errors (e.g., KeyError/FunctionClauseError) and potentially return `{:ok, session}` even when `mark_opening` crashed for an unrelated reason. Consider rescuing only the specific Ash/state-machine transition errors that indicate the session already moved past `:requested`, and re-raising (or returning `{:error, error}` without recovery) for anything else.

Copilot commented

2026-04-01 00:35:04 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR review comment.

Original author: @Copilot
Original URL: https://github.com/carverauto/serviceradar/pull/3109#discussion_r3019139716
Original created: 2026-04-01T00:35:04Z
Original path: elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex
Original line: 236

The warning log includes inspect(reason). When reason is an Ash error/changeset it can include request attributes (e.g., lease token / command id) and may leak sensitive data into logs. Consider logging only the exception module/message (or a sanitized subset) and attach structured metadata instead of inspect/1 on the whole error term.

Imported GitHub PR review comment. Original author: @Copilot Original URL: https://github.com/carverauto/serviceradar/pull/3109#discussion_r3019139716 Original created: 2026-04-01T00:35:04Z Original path: elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex Original line: 236 --- The warning log includes `inspect(reason)`. When `reason` is an Ash error/changeset it can include request attributes (e.g., lease token / command id) and may leak sensitive data into logs. Consider logging only the exception module/message (or a sanitized subset) and attach structured metadata instead of `inspect/1` on the whole error term.

Copilot commented

2026-04-01 00:35:04 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR review comment.

Original author: @Copilot
Original URL: https://github.com/carverauto/serviceradar/pull/3109#discussion_r3019139730
Original created: 2026-04-01T00:35:04Z
Original path: elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex
Original line: 117

If mark_session_opening_safely/7 returns {:error, reason} (i.e., it couldn't recover by reloading an :opening/:active session), request_open/3 currently just returns that error and leaves the newly-created relay session in :requested without marking it failed or emitting a health failure event. Consider handling the non-recoverable {:error, reason} case similarly to the dispatch failure path (call mark_failed and maybe_record_session_failure) to avoid orphaned/stuck sessions.

Imported GitHub PR review comment. Original author: @Copilot Original URL: https://github.com/carverauto/serviceradar/pull/3109#discussion_r3019139730 Original created: 2026-04-01T00:35:04Z Original path: elixir/serviceradar_core/lib/serviceradar/camera/relay_session_manager.ex Original line: 117 --- If `mark_session_opening_safely/7` returns `{:error, reason}` (i.e., it couldn't recover by reloading an `:opening`/`:active` session), `request_open/3` currently just returns that error and leaves the newly-created relay session in `:requested` without marking it failed or emitting a health failure event. Consider handling the non-recoverable `{:error, reason}` case similarly to the dispatch failure path (call `mark_failed` and `maybe_record_session_failure`) to avoid orphaned/stuck sessions.