Wifi aware refactor mesh core #545
Conversation
* count peers in notification * cleanup
# Conflicts: # app/src/main/res/values-ar/strings.xml # app/src/main/res/values-bn/strings.xml # app/src/main/res/values-de/strings.xml # app/src/main/res/values-es/strings.xml # app/src/main/res/values-fa/strings.xml # app/src/main/res/values-fil/strings.xml # app/src/main/res/values-fr/strings.xml # app/src/main/res/values-he/strings.xml # app/src/main/res/values-hi/strings.xml # app/src/main/res/values-id/strings.xml # app/src/main/res/values-it/strings.xml # app/src/main/res/values-ja/strings.xml # app/src/main/res/values-ka/strings.xml # app/src/main/res/values-ko/strings.xml # app/src/main/res/values-mg/strings.xml # app/src/main/res/values-ms/strings.xml # app/src/main/res/values-ne/strings.xml # app/src/main/res/values-nl/strings.xml # app/src/main/res/values-pa-rPK/strings.xml # app/src/main/res/values-pl/strings.xml # app/src/main/res/values-pt-rBR/strings.xml # app/src/main/res/values-pt/strings.xml # app/src/main/res/values-ru/strings.xml # app/src/main/res/values-sv/strings.xml # app/src/main/res/values-ta/strings.xml # app/src/main/res/values-th/strings.xml # app/src/main/res/values-tr/strings.xml # app/src/main/res/values-uk/strings.xml # app/src/main/res/values-ur/strings.xml # app/src/main/res/values-vi/strings.xml # app/src/main/res/values-zh-rCN/strings.xml # app/src/main/res/values-zh-rTW/strings.xml # app/src/main/res/values-zh/strings.xml
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5983d700cd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!isActive) return | ||
| isActive = false | ||
| announceJob?.cancel() | ||
| announceJob = null | ||
| gossipSyncManager.stop() |
There was a problem hiding this comment.
When WifiAwareMeshService is constructed it passes MeshServiceHolder.sharedGossipSyncManager into MeshCore, which is the BLE instance’s gossip manager. stopCore() always calls gossipSyncManager.stop(), so stopping Wi‑Fi will also stop BLE’s gossip sync/cleanup loops even if the BLE service is still running. In practice this means disabling Wi‑Fi Aware (or losing Aware permissions) can silently halt BLE gossip synchronization until BLE is restarted. Consider only starting/stopping the manager when the MeshCore owns it, or add reference counting for shared managers.
Useful? React with 👍 / 👎.
* Refactor WifiAware memory management and lifecycle handling * Fix Wi-Fi Aware reconnection by ensuring callback unregistration and session watchdog * fix(wifiaware): allow WifiAwareController to restart service if session drops * Enhance WiFi Aware logging with network request timeouts and detailed callback status * Ensure explicit release of WiFi Aware network callbacks on failure or disconnection --------- Co-authored-by: aidenvalue <>
* fix(wifi-aware): restore peer check and remove aggressive session restarts * synchronized access to sockets. * clear old peer socket `onClientConnected` if one was found. * address second round of review * fix: Restart publish session on termination in WifiAwareMeshService * fix: cleanup connection tracker resources when handling peer disconnection without active socket --------- Co-authored-by: aidenvalu3 <>
callebtc
left a comment
callebtc
left a comment
There was a problem hiding this comment.
Great work on abstracting the Mesh logic! The MeshCore design is much cleaner and allows for easier addition of new transports.
However, I found a few critical issues regarding lifecycle management and data persistence that need to be addressed before merging.
Critical Issues
-
Memory Leak in MainActivity
InMainActivity.kt, you set the delegate forWifiAwareMeshServiceinside alifecycleScope:svc.delegate = object : com.bitchat.android.wifiaware.WifiAwareMeshDelegate { ... }
However, unlike
meshService.delegate = nullinonPause(), you never nullify theWifiAwareMeshServicedelegate. SinceWifiAwareControllerholds a singleton reference to the service, and the anonymous delegate holds a reference toMainActivity(andChatViewModel), this causes theMainActivityto leak every time it is destroyed/recreated. -
Data Persistence & Background Handling
You are handling message persistence for Wi-Fi Aware inside theMainActivitydelegate:if (message.isPrivate) { AppStateStore.addPrivateMessage(...) }
This is risky because:
- If the UI is not active (app in background),
MainActivitymight be paused/stopped. If we fix the memory leak by clearing the delegate inonPause, Wi-Fi Aware messages received in the background will be lost (not saved to DB), becauseWifiAwareMeshServiceitself doesn't save them. BluetoothMeshServicehandles this correctly by passing a hook toMeshCore(onMessageReceived = { ... AppStateStore ... }).- Fix: Move the
AppStateStorepersistence logic intoWifiAwareMeshService(passing it as a hook to itsMeshCoreinstance), exactly like it is done inBluetoothMeshService. This ensures messages are saved regardless of UI state and removes code duplication fromMainActivity.
- If the UI is not active (app in background),
Architectural Observations
-
Split Brain Peer State
You have twoMeshCoreinstances (BLE and Wi-Fi), each with its ownPeerManager. The UI (MeshDelegateHandler) merges these lists. This works, but it meansBluetoothMeshServicedoesn't inherently know about Wi-Fi peers (only via the bridge).- Make sure
TransportBridgeServicerouting is robust enough that ifBluetoothMeshServicereceives a packet for a Wi-Fi peer, it correctly routes it to the bridge (which it seems to do viadispatchGlobal). - Suggestion: Long-term, a single
MeshCoremanaging multipleTransportinstances might be cleaner, but the current approach of bridging two services is a valid "micro-service" style approach within the app.
- Make sure
-
Shared Gossip Manager Race Condition
val shared = com.bitchat.android.service.MeshServiceHolder.sharedGossipSyncManager meshCore = MeshCore(..., sharedGossipManager = shared, ...)
If
WifiAwareMeshServicestarts beforeBluetoothMeshService(e.g., race condition inMainActivityinit or permission grants),sharedwill be null.WifiAwareMeshServicewill create its own. ThenBluetoothMeshServicestarts and creates another one.- Fix: Ensure
MeshServiceHolderinitializes theGossipSyncManagerlazily or explicitly before either service starts, so they definitely share the same instance.
- Fix: Ensure
Code Style / Minor
- Duplication: The code in
MainActivityto save messages is a copy-paste of logic that should be in the service layer. Moving it toWifiAwareMeshServicehooks fixes this. - MeshCore Hooks: The
Hookspattern is flexible, butWifiAwareMeshServiceignores theonMessageReceivedhook, leading to the issue above.
Please address these points, especially the memory leak and the background persistence issue.
…Wi-Fi Aware - Move message persistence and background notifications for Wi-Fi Aware into the service layer via MeshCore hooks. - Fix memory leak in MainActivity by properly detaching WifiAwareMeshDelegate on pause. - Ensure BluetoothMeshService is initialized before WifiAwareMeshService to guarantee shared GossipSyncManager instance. - Deduplicate persistence logic from MainActivity delegate.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 689f3db1f9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (sent) { | ||
| // Ensure unread badge is cleared for this peer immediately | ||
| try { | ||
| val current = state.getUnreadPrivateMessagesValue().toMutableSet() | ||
| if (current.remove(senderPeerID)) { | ||
| state.setUnreadPrivateMessages(current) | ||
| } |
There was a problem hiding this comment.
Because unread badges are cleared only when sent is true, a user who opens a focused chat while the peer is offline/no session will keep seeing the conversation marked unread even though they just read it. The read receipt may legitimately fail to send (no direct connection or session), but local read state should still be cleared to avoid stuck unread indicators until the peer reconnects. Consider clearing the unread set unconditionally and optionally queueing the read receipt separately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 689f3db1f9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (_running.value) { | ||
| if (!_enabled.value) return | ||
| // If already marked running but we are calling this (e.g. after a drop), | ||
| // ensure we clean up the old service before restarting. | ||
| stop() |
There was a problem hiding this comment.
startIfPossible() now stops the service whenever _running is true, so any caller that uses it as an “ensure running” helper will tear down existing Wi‑Fi Aware sessions. Since MainActivity.onResume() now calls WifiAwareController.startIfPossible() when location is enabled, simply backgrounding/foregrounding the app will drop active connections and reset in‑flight transfers. Consider making this path a no‑op when already running (or only restarting after an explicit failure signal) to avoid unnecessary disconnects.
Useful? React with 👍 / 👎.
|
Critical Issues
// WifiAwareMeshService relies on BluetoothMeshService being initialized first
When does peerSockets get cleaned up on disconnect? Need to verify all paths:
TransportBridgeService.broadcast() calls all transports synchronously. If Wi-Fi is slow, BLE packets queue unbounded.
Both BLE and Wi-Fi have separate MeshCore instances. Packet arriving via bridge could be processed twice. Need to verify deduplication works across cores. |
Description
This PR refactors the mesh networking layer to introduce
MeshCore, a shared coordinator that abstracts common mesh logic (peer management, security, packet processing, etc.) away from specific transports. This enables multi-transport support (BLE + Wi-Fi Aware) with shared state and seamless routing between them.Key Changes:
PeerManager,SecurityManager,StoreForwardManager, andPacketProcessorinto a reusableMeshCoreclass.BluetoothMeshServiceandWifiAwareMeshServicenow utilizeMeshCoreto handle protocol-level logic while focusing on their respective hardware transport implementations.TransportBridgeServiceto route packets between different transport layers.GossipSyncManagerinstance viaMeshServiceHolder.Critical Fixes:
MainActivitynow properly detaches itsWifiAwareMeshDelegateinonPauseto prevent leaking the Activity context through the singletonWifiAwareController.MeshCore.Hooks. This ensures messages are saved toAppStateStoreand notifications are shown even when the UI is closed.BluetoothMeshServiceis initialized beforeWifiAwareMeshServiceto ensure a consistent source for the sharedGossipSyncManager.MainActivitycallbacks.Checklist