fix(kvm): close browser websocket immediately on AMT disconnect#820
fix(kvm): close browser websocket immediately on AMT disconnect#820nmgaston wants to merge 2 commits intodevice-management-toolkit:mainfrom
Conversation
9ac8e2b to
7361bb8
Compare
6733334 to
5b28824
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves KVM/SOL/IDER session teardown behavior by proactively closing the browser WebSocket as soon as the AMT/device side disconnects, preventing the UI from appearing “frozen” while ListenToBrowser remains blocked.
Changes:
- Updated
ListenToDevicecleanup to send a WebSocket close frame to the browser and close the connection immediately on exit. - Added a private unit test to verify
CloseMessage+Close()are invoked when the AMT side drops.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
internal/usecase/devices/interceptor.go |
Sends CloseMessage + closes browser WebSocket in ListenToDevice defer; minor comment tweak near length casting. |
internal/usecase/devices/interceptor_private_test.go |
Adds a spy-based unit test asserting the browser WebSocket is closed when AMT disconnects. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _ = deviceConnection.Conn.WriteMessage( | ||
| websocket.CloseMessage, | ||
| websocket.FormatCloseMessage(websocket.CloseNormalClosure, "AMT session ended"), | ||
| ) | ||
| _ = deviceConnection.Conn.Close() |
There was a problem hiding this comment.
In ListenToDevice, the function writes device data using the locally captured conn := deviceConnection.Conn, but the new deferred cleanup closes deviceConnection.Conn directly. Since deviceConnection.Conn can be updated elsewhere (e.g., reconnect path), this can lead to closing a different WebSocket than the one used for writes (and potentially leaving the original connection open). Consider using a single, consistent reference for both writing and cleanup (e.g., close conn, or access/update Conn under a lock and always use the current value).
| _ = deviceConnection.Conn.WriteMessage( | |
| websocket.CloseMessage, | |
| websocket.FormatCloseMessage(websocket.CloseNormalClosure, "AMT session ended"), | |
| ) | |
| _ = deviceConnection.Conn.Close() | |
| if conn != nil { | |
| _ = conn.WriteMessage( | |
| websocket.CloseMessage, | |
| websocket.FormatCloseMessage(websocket.CloseNormalClosure, "AMT session ended"), | |
| ) | |
| _ = conn.Close() | |
| } |
| } | ||
|
|
||
| length := uint32(totalLength) //nolint:gosec // Ignore potential integer overflow here as overflow is validated earlier in code | ||
| length := uint32(totalLength) // overflow validated above |
There was a problem hiding this comment.
length := uint32(totalLength) will likely be flagged by the enabled gosec linter (integer conversion) even though there's a bounds check above. Since this file already uses //nolint:gosec for similar validated conversions, consider restoring a //nolint:gosec on this line or refactoring the conversion in a way that satisfies gosec so CI doesn't fail.
| length := uint32(totalLength) // overflow validated above | |
| length := uint32(totalLength) //nolint:gosec // overflow validated above via explicit bounds check |
| cancel: cancel, | ||
| healthTicker: time.NewTicker(HeartbeatInterval), | ||
| } | ||
|
|
There was a problem hiding this comment.
This test allocates a ticker and a cancellable context but never stops/cancels them. Even though it’s a unit test, it’s better to avoid leaking timers/resources (and it helps with leak-detecting tooling). Consider using t.Cleanup to stop the ticker and call cancel(), or omit healthTicker entirely if it isn’t needed for this test.
| t.Cleanup(func() { | |
| cancel() | |
| deviceConnection.healthTicker.Stop() | |
| }) |
NOTE: Use with sample-web-ui PR: device-management-toolkit/sample-web-ui#3159
Problem
When an AMT device disconnects (e.g. the device is powered off or the network drops), the KVM/SOL/IDER session would appear frozen in the browser UI. The browser websocket was only closed after
ListenToBrowserunblocked from itsReadMessagecall, which could take up to 30 seconds (the inactivity timeout) or indefinitely if the browser wasn't sending data.Fix
In
ListenToDevice, the deferred cleanup now immediately sends a websocketCloseMessageto the browser and closes the connection before cancelling the context. This means the browser receives the close frame as soon as the AMT device side drops, allowing the UI to update without waiting forListenToBrowserto unblock.Changes
internal/usecase/devices/interceptor.go— updated theListenToDevicedefer to sendwebsocket.CloseMessageand callClose()on the browser connection beforecancel()internal/usecase/devices/interceptor_private_test.go— addedTestListenToDeviceClosesWebSocketOnAMTDisconnectto verifyWriteMessage(CloseMessage)andClose()are called on the browser websocket when the AMT side disconnects