develop -> main - #114
Closed
swethasukumarr wants to merge 14 commits into
Closed
Conversation
Chore: one time sync of main -> develop to prepare for ff automation
RDKEMW-19316 : Add coding guidelines for firebolt-cpp-transport
…cation * Replace SubscriptionData value map with shared_ptr; drop stale notifications via weak_ptr * Address copilot comments * Address copilot comments * Catch any_cast errors in onPropertyChangedCallback * Address copilot comments
RDKEMW-22177: update automation to use new PAT
dhillomk
approved these changes
Jul 22, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR syncs develop into main, bringing in stability fixes around event-notification delivery (preventing crashes from stale subscription state / callback exceptions), plus CI automation credential updates and a migration of Copilot guidance into a consolidated coding-guidelines document.
Changes:
- Hardened subscription notification delivery by guarding helper subscription callbacks with a
weak_ptr, avoiding use-after-free during unsubscribe races. - Made the gateway notification worker resilient to exceptions thrown by callbacks, and added regression tests covering both the weak_ptr guard and exception handling.
- Updated GitHub Actions workflows to use
secrets.AUTOMATION_TOKEN, and replaced the previous Copilot instructions file with a larger coding-guidelines instructions doc.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/helpers_impl.h |
Stores subscriptions as shared_ptr and wraps gateway callbacks with a weak_ptr guard to drop in-flight notifications safely. |
include/firebolt/helpers.h |
Improves robustness of event property-change callback by catching std::bad_any_cast and refining log text. |
src/gateway.cpp |
Catches exceptions in the notification worker dispatch loop so a single bad callback doesn’t terminate the worker thread. |
test/unit/helperTest.cpp |
Adds regression tests reproducing the unsubscribe/use-after-free race and validating the weak_ptr guard behavior. |
test/unit/gatewayTest.cpp |
Adds a regression test ensuring the notification worker continues after a callback throws. |
.github/workflows/sync-develop-to-main.yml |
Switches automation auth to secrets.AUTOMATION_TOKEN. |
.github/workflows/release.yml |
Switches release auth to secrets.AUTOMATION_TOKEN. |
.github/instructions/coding-guidelines.instructions.md |
Adds consolidated repo coding/testing/build guidelines as an instructions document. |
.github/copilot-instructions.md |
Removed in favor of the new consolidated guidelines document. |
Comment on lines
19
to
+23
| #include "firebolt/gateway.h" | ||
| #include "firebolt/helpers.h" | ||
| #include <map> | ||
| #include <memory> | ||
| #include <mutex> |
Comment on lines
+82
to
85
| void* notificationPtr = static_cast<void*>(it->second.get()); | ||
| auto errorStatus{gateway_.unsubscribe(it->second->eventName, notificationPtr)}; | ||
| subscriptions_.erase(it); | ||
| return Result<void>{errorStatus}; |
Comment on lines
121
to
125
| uint64_t newId = currentId_++; | ||
| subscriptions_[newId] = SubscriptionData{owner, eventName, std::move(notification)}; | ||
| void* notificationPtr = reinterpret_cast<void*>(&subscriptions_[newId]); | ||
| auto spData = std::make_shared<SubscriptionData>(SubscriptionData{owner, eventName, std::move(notification)}); | ||
| subscriptions_[newId] = spData; | ||
| void* notificationPtr = static_cast<void*>(spData.get()); | ||
|
|
Comment on lines
+609
to
+618
| // --------------------------------------------------------------------------- | ||
| // Race condition fix tests (MONUI-908) | ||
| // | ||
| // These tests validate the weak_ptr guard introduced in HelperImpl::subscribe. | ||
| // The guard prevents a use-after-free crash that occurred when: | ||
| // 1. The platform sent an event notification (e.g. onSpeechInterrupted). | ||
| // 2. Before the worker thread dispatched it, unsubscribeAll() ran and | ||
| // destroyed the SubscriptionData object. | ||
| // 3. The worker thread then invoked the registered callback with a dangling | ||
| // void* pointer → bad_any_cast → std::terminate → device crash. |
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.
No description provided.