Rdkemw 17528 02#947
Merged
Merged
Conversation
…ebSocket disconnect *Sessions were registered by connection_id but cleanup used session_id,causing remove_session() to be a no-op. *Event listeners and empty map entries accumulated indefinitely across disconnects. Changes: - app_events.rs: Add cleanup_by_connection_id(), prune empty entries in remove_session() - firebolt_gateway.rs: Clean up by both session_id and connection_id on UnregisterSession - thunder_event_processor.rs: Add cleanup_by_app_id() for extension-side event map cleanup - firebolt_ws.rs: Elevate disconnect log from debug! to info! with full context
…ventProcessor Subscription entries. -In request_map and extension_request_map were never removed on disconnect. -ThunderEventProcessor.cleanup_by_app_id() existed but was never called. -Added DeviceEvent::Cleanup to wire ThunderEventProcessor cleanup across the gateway-extension boundary. -Added cleanup_request_maps_for_app() to prune broker maps on disconnect.
There was a problem hiding this comment.
Pull request overview
This PR adds disconnect-time cleanup to reduce leaked listeners/subscriptions across Firebolt app events, broker request tracking, and Thunder device event handling.
Changes:
- Add a
DeviceEvent::Cleanuprequest that is sent on session disconnect to purge Thunder-side device event listeners for the disconnected app. - Improve in-process cleanup by pruning empty
AppEventslistener maps and adding cleanup byconnection_id. - Enhance broker cleanup by removing stale request tracking entries and improving Thunder broker unsubscribe/unregister behavior on disconnect.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| device/thunder_ripple_sdk/src/processors/thunder_events.rs | Handles new DeviceEvent::Cleanup request in the Thunder extension processor. |
| device/thunder_ripple_sdk/src/events/thunder_event_processor.rs | Adds cleanup_by_app_id plus unit tests for removing per-app listeners. |
| core/sdk/src/api/device/device_events.rs | Introduces DeviceEvent::Cleanup, parsing, contract mapping, and tests. |
| core/main/src/service/apps/app_events.rs | Prunes empty listener maps on session removal; adds cleanup by connection_id and tests. |
| core/main/src/firebolt/firebolt_ws.rs | Logs richer disconnect information before unregistering the session. |
| core/main/src/firebolt/firebolt_gateway.rs | Performs broader disconnect cleanup (events, broker, endpoint state) and triggers Thunder cleanup request. |
| core/main/src/broker/thunder_broker.rs | Improves broker cleaner unsubscribe behavior and channel sizing; adds warnings/logging. |
| core/main/src/broker/endpoint_broker.rs | Adds request/extension map cleanup on disconnect and unit tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…load Change try_send to send().await in BrokerCleaner::cleanup_session() to wait for channel capacity instead of silently dropping cleanup messages when the channel is full, which prevented Thunder .unregister calls from being sent.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
ID in the forwarder loop and skip event_handler processing when payload is unchanged. Adds 5 unit tests for the dedup logic.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… events" This reverts commit e9d824a.
Each call to jq_compile() created Rc reference cycles in jaq-interpret that were never freed, leaking ~120 heap nodes (~24 KB) per event_handler cycle. fix: Cache compiled Filter objects in a static HashMap so each unique filter string is parsed and compiled only once. Subsequent calls clone the cached filter (cheap Rc refcount increment) instead of recompiling.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Rename cleanup_request_maps_for_app to cleanup_request_maps and parameter app_id to id to clarify it may be app_id, session_id, or connection_id - Drop event_map write lock before acquiring last_event write lock in cleanup_by_app_id to prevent potential deadlocks
Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
Minimum allowed line rate is |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tfarz123
approved these changes
Apr 29, 2026
Vinodsathyaseelan
approved these changes
Apr 29, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
What does this PR add or remove?
Why
Why are these changes needed?
How
How do these changes achieve the goal?
Test
How has this been tested? How can a reviewer test it?
Checklist