Skip to content

feat: add Stop-DeviceApp command - #55

Open
JoshuaMoelans wants to merge 2 commits into
mainfrom
joshua/feat/switch-terminate
Open

feat: add Stop-DeviceApp command#55
JoshuaMoelans wants to merge 2 commits into
mainfrom
joshua/feat/switch-terminate

Conversation

@JoshuaMoelans

Copy link
Copy Markdown
Member

As part of https://github.com/getsentry/sentry-switch/pull/165 where we noticed that Integration Test on Switch Ounce are failing, because the events stay Pending.

This is a more robust follow-up of what we introduced in https://github.com/getsentry/sentry-switch/pull/144 , where we invoked terminate for the unreal demo. Will follow up from this PR in the 165 revert³ one on sentry-switch.

[CmdletBinding()]
param()

Assert-DeviceSession

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The Assert-DeviceSession health check may throw an exception on a crashed device, preventing the StopApplication() command from running and defeating the script's purpose.
Severity: HIGH

Suggested Fix

Either modify Stop-DeviceApp to skip or handle the exception from the Assert-DeviceSession check, or change Assert-DeviceSession to use a more robust health check that will not fail in post-crash scenarios.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: app-runner/Public/Stop-DeviceApp.ps1#L22

Potential issue: The `Stop-DeviceApp` script is intended to function after an
application crash. However, it calls `Assert-DeviceSession` on line 22, which performs a
health check by running a `getstatus` command. If a device becomes unresponsive to
`getstatus` after a crash—the exact scenario the script is designed for—the health check
will throw an exception. This exception prevents the script from ever reaching the
`$provider.StopApplication()` call on line 27, defeating the purpose of the new
functionality.

Did we get this right? 👍 / 👎 to inform future reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant