[WIP] Avatar persistence and synchronization #248

jolavillette · 2026-01-22T17:27:24Z

Code and comment by Gemini

This summary describes the technical implementation of the new avatar persistence and synchronization system for RetroShare's Chat Service.

Core Implementation Overview
The goal of these changes was to provide a reliable, persistent, and proactive avatar exchange mechanism that operates silently in the background without interrupting the user experience or breaking existing services like Chat Lobbies.

Persistence and Data Integrity
To ensure avatars survive application restarts, the storage logic was migrated to a Key-Value set system within the configuration.

saveList: Now encodes the local node's avatar and all cached peer avatars into RsConfigKeyValueSet items using Radix64 encoding. It explicitly calls parent services to ensure Chat Lobby subscriptions are also saved.

loadList: Implements a non-destructive "Pass-through" logic. It extracts avatar data from KV sets but relays any unrecognized items to DistributedChatService::processLoadListItem and DistantChatService::processLoadListItem.

Lobby Protection: This delegation ensures that shared configuration containers containing lobby data or GXS identities are not deleted by the Chat Service during the loading phase.

Real-time Synchronization & Handshaking
The system now proactively ensures that avatars are up-to-date across the network through two main triggers:

Broadcast on Update (setOwnNodeAvatarData): When the user changes their avatar, the service immediately broadcasts the new image data to all currently online peers.

Handshake on Connection (statusChange): As soon as a peer connects, a bidirectional handshake is initiated. The service sends an avatarRequest to the peer and simultaneously pushes the local avatar data to them.

Silent Protocol: These exchanges use direct RsChatAvatarItem packets, which are processed by the remote UI in the background, preventing unnecessary chat window popups.

Thread Safety and Stability
A critical deadlock identified through GDB debugging was resolved to ensure system stability.

Deadlock Prevention: The RsStackMutex was removed from makeOwnAvatarItem to prevent recursive locking when called from parent functions that already hold the mChatMtx lock.

Scoped Locking: In setOwnNodeAvatarData, the mutex is now confined to a specific scope to ensure it is released before entering the network broadcast loop, allowing child functions to acquire the lock safely.

csoler · 2026-01-24T10:40:01Z

src/chat/p3chatservice.cc


-	mChatMtx.lock(); /****** MUTEX LOCKED *******/
+	/* 2. Original developer items (Messages & Status) */
+	mChatMtx.lock(); 


I don't like these .lock() (I know it was here before). It's probably a good opportunity to replace it with a stack mutex.

csoler · 2026-01-24T10:42:44Z

src/chat/p3chatservice.cc

-		ci->PeerId(mServiceCtrl->getOwnId());
+		RsConfigKeyValueSet *item = new RsConfigKeyValueSet();
+		RsTlvKeyValue kv; kv.key = "AV_CACHE:OWN";
+		Radix64::encode(_own_avatar->_image_data, _own_avatar->_image_size, kv.value);


Note: RsConfigKeyValueSet is rather limiting in the type of what would be used (basically strings), which explains the Radix64 format. You would get a smaller print using an explicit encoding in the RsItem, but this will cause problems of backward compatibility.

csoler · 2026-01-24T11:42:41Z

src/chat/p3chatservice.cc

 RsChatAvatarItem *p3ChatService::makeOwnAvatarItem()
 {
-	RsStackMutex stack(mChatMtx); /********** STACK LOCKED MTX ******/
+	/* Mutex is removed here to prevent recursive deadlock from parent calls */


Good practice: if you remove the mutex, rename the method as locked_makeOwnAvatarItem() : this indicates that the method should be called within a mutex-protected region.

csoler · 2026-01-24T11:47:52Z

src/chat/p3chatservice.cc

+	RsDbg() << "AVATAR entering loadList, total items: " << (int)load.size();

-	for(std::list<RsItem*>::const_iterator it(load.begin());it!=load.end();++it)
+	for(std::list<RsItem*>::iterator it(load.begin()); it != load.end(); )


Since you changed the way your own avatar is saved, this is not backward compatible: patched users will lose their avatar and will have to re-define it.

I propose a better alternative: use RsChatAvatarItem as before, setting the PeerId() of the item to the node ID of that avatar. This is backward compatible with what's done now, and also will require less storage since it's not converting to Radix64.

csoler · 2026-01-24T11:48:34Z

src/chat/p3chatservice.cc

+			RsChatStatusItem *mitem = dynamic_cast<RsChatStatusItem *>(item);
+			if(mitem)
+			{
+				_custom_status_string = mitem->status_string ;


why remove the mutex?

csoler · 2026-01-24T11:52:46Z

Use RsChatAvatarItem instead of KeyValueSet => it will be backward compatible and the code is also simpler.

jolavillette · 2026-01-24T17:03:26Z

Following Cyril comments:

Own avatar is saved as a RsChatAvatarItem (existing logic confirmed/preserved), insuring backward compatibility. Saved ID is 0, and is interpreted as our ID, see below.
For peers the legacy RsChatAvatarItem could not be used because its serialization ignores the Peer ID (saving only image data), causing lost IDs (zero) on reload. Implemented custom saving using RsConfigKeyValueSet to persist both Peer ID and Avatar Data. Avatars are saved in chunks (10 items per key-value set) to respect serializer size limits and ensure reliable writing to disk.
Various code improvements

csoler · 2026-01-27T17:21:12Z

src/chat/p3chatservice.cc

-   }
-   _avatars[ci->PeerId()] = new AvatarInfo(ci->image_data,ci->image_size) ; 
-   _avatars[ci->PeerId()]->_peer_is_new = true ;
-   _avatars[ci->PeerId()]->_own_is_new = new_peer ;


How do you keep track of peers to which we haven't sent our own avatar yet?

csoler · 2026-01-27T17:22:14Z

src/chat/p3chatservice.cc

 	uint32_t s = 0 ;
 #ifdef CHAT_DEBUG
-	std::cerr << "p3chatservice:: own avatar requested from above. " << std::endl ;
+	//std::cerr << "p3chatservice:: own avatar requested from above. " << std::endl ;


I assume you left CHAT_DEBUG on. Make sure all debug output is disabled/cleaned before we merge.

csoler · 2026-01-27T17:41:27Z

src/chat/p3chatservice.cc

+        }
+
+        if(kv != NULL) {
+            if(!kv->tlvkvs.pairs.empty())


this check is not necessary (following the code above it's not possible to have an item with an empty list). It's of course not a problem to keep it.

csoler · 2026-01-27T17:46:30Z

src/chat/p3chatservice.cc

+
+			/* AVATAR Handshake on connection */
+			RsDbg() << "AVATAR peer connected, initiating sync with: " << it->id.toStdString().c_str();
+			sendAvatarRequest(it->id);


This is going to cause sending the avatar at every connection. To make it reliable we should send the TS of the avatar we have, and if the peer has a more recent avatar then it should send it. Looking at the code in sendAvatarRequest() it should only be used if we do nto have the avatar.

csoler · 2026-01-27T17:46:49Z

src/chat/p3chatservice.cc

+			sendAvatarRequest(it->id);
+			if(_own_avatar != nullptr && _own_avatar->_image_size > 0)
+			{
+				sendAvatarJpegData(it->id);


same here. Why would we send our own avatar without a request?

jolavillette · 2026-01-27T21:17:41Z

New packet type: Added RsChatAvatarInfoItem containing only the avatar timestamp (replaces systematic full image broadcasting)
Connection handshake: On peer connection, we now send [sendAvatarInfo()] (timestamp only) instead of [sendAvatarJpegData()] (full image)
Smart pull mechanism:
- Peer receives timestamp and compares with local cache
- If remote timestamp is newer → peer sends [sendAvatarRequest()]
- If timestamp is same/older → no action (bandwidth saved)
Avatar updates: When user changes their avatar, the new timestamp is broadcast to all connected peers, triggering the same pull mechanism
Debug cleanup: Disabled CHAT_DEBUG define (debug blocks kept for future debugging)

jolavillette · 2026-01-28T09:57:41Z

Save/restore of avatars is broken since last commit (TS). Stand by for a fix.

jolavillette · 2026-01-28T14:16:23Z

Fix Avatar Persistence Regression

Fixes avatar persistence issue introduced by timestamp sync implementation.

Problem: Avatars were not persisting across sessions because _timestamp was not updated when receiving avatar data.

Solution:

Update timestamp in AvatarInfo::init() when image data is set
Request peer avatars on connection if not cached (backward compatibility)

Result: Avatars now persist correctly and work with both old and new nodes.

…dlock

…t missing avatar on peer connection for backward compatibility

csoler

Sorry for the new requested change, but I really think it would be for the best.

csoler · 2026-02-06T16:00:28Z

src/chat/p3chatservice.cc

+    /* Fix: Constructor for combined TS + Radix64 loading */
+    AvatarInfo(const std::string& encoded_data) : _image_size(0), _image_data(NULL)
+    {
+        if (encoded_data.length() > 16)


This way of doing things is pretty dangerous: you may get a full radix64 string that unfortunately starts with digits and therefore interpret it the wrong way.

All this is because of the key/value storage in SaveList(). I would be much simpler/safer to use a RsItem instead. I suggest that you create a new RsItem type (say RsAvatarInfoConfigItem) that includes the TS of the avatar, the owner of the avatar, and the avatar as RsTlvImage: this way no need for specific serialization (the serializer does it automatically).

You can either keep that structure as the holder of the info (after loading it) meaning that in this case you do not destroy the avatar item that you just got in LoadList, or convert it to an AvatarInfo structure as done currently.

Since this new avatar item type can be used for all peers including yourself, it makes the saving more consistent. In LoadList, just keep the loading for the original avatar item for backward compatibility.

…persistence

jolavillette · 2026-02-07T20:51:50Z

Description

This commit refactors the avatar persistence mechanism in [p3ChatService] to address safety concerns regarding manual string parsing.
Previously, avatars were saved by concatenating a timestamp and the Radix64-encoded image into a single string. This method was fragile and prone to parsing errors if the data format wasn't strictly followed.

Changes

Introduced [RsChatAvatarConfigItem] (subtype RS_PKT_SUBTYPE_CHAT_AVATAR_CONFIG) in [rschatitems.h] to store:
- peerId
- timestamp
- image_data
Implemented standard serialization for this item using RsTypeSerializer.
Updated p3ChatService::saveList to save peer avatars using this new item instead of the old key-value pair method.
Updated p3ChatService::loadList to handle the new item while maintaining backward compatibility for reading existing configurations (old [RsChatAvatarItem] and Key-Value sets).

Verification

Verified that avatars are correctly saved to [config] with the new format.
Verified that avatars are correctly restored upon restart.
Verified that avatars from older configurations are still loaded correctly (backward compatibility).

csoler · 2026-02-08T16:39:56Z

src/chat/p3chatservice.cc

+    }
+
+    /* Fix: Returns TS (16 hex chars) + Radix64 image data */
+    std::string toRadix64() const


This one is not used anymore now, so you can remove it.

csoler · 2026-02-08T16:40:12Z

src/chat/p3chatservice.cc

+    ~AvatarInfo() { if (_image_data) free(_image_data); }
+
+    /* Fix: Constructor for combined TS + Radix64 loading */
+    AvatarInfo(const std::string& encoded_data) : _image_size(0), _image_data(NULL)


This constructor is not used anymore: remove it.

csoler · 2026-02-08T17:38:34Z

src/chat/p3chatservice.cc

-			    _avatars[pid] = new AvatarInfo(mit->value);
+			    if (_avatars.count(pid)) delete _avatars[pid];
+
+                /* Backward compatibility: decode KVS string (TS prefix + Radix64) */


I think you can also remove that part: missing avatars will be transferred anyway, and this code is a security hole.

jolavillette force-pushed the AvatarStorageAndUpdateImprove branch from 93fe7c5 to f7273a3 Compare January 24, 2026 07:58

csoler reviewed Jan 24, 2026

View reviewed changes

jolavillette force-pushed the AvatarStorageAndUpdateImprove branch 2 times, most recently from ba0143c to 9a9813e Compare January 27, 2026 06:29

csoler reviewed Jan 27, 2026

View reviewed changes

jolavillette force-pushed the AvatarStorageAndUpdateImprove branch from 0242ad7 to 5bece81 Compare January 27, 2026 21:21

defnax mentioned this pull request Jan 28, 2026

[Qt] Friends Avatars needs to be cached/stored RetroShare/RetroShare#1706

Closed

jolavillette added 8 commits February 6, 2026 15:30

implement persistence for remote peer avatars using KV sets

537fcf6

initiate proactive avatar exchange on peer connection

07a2808

implement direct avatar broadcast on update and on connection

cdb3769

restore lobby persistence via parent delegation and fix recursive dea…

8b69ffc

…dlock

code and logic improvement following cyril comment

468a9e1

Implement avatar timestamp handshake to reduce bandwidth usage

a4f2c0e

fix persistence by updating TS when an avatar is received, and reques…

240a5c6

…t missing avatar on peer connection for backward compatibility

remove AVATAR debug messages

e1fee08

jolavillette force-pushed the AvatarStorageAndUpdateImprove branch from 5608ae9 to e1fee08 Compare February 6, 2026 14:35

csoler reviewed Feb 6, 2026

View reviewed changes

replace custom string parsing with RsChatAvatarConfigItem for avatar …

8371060

…persistence

unify own avatar persistence using RsChatAvatarConfigItem

50209e6

csoler reviewed Feb 8, 2026

View reviewed changes

remove unused AvatarInfo methods and inline legacy decoding logic

92ff068

csoler reviewed Feb 8, 2026

View reviewed changes

remove legacy avatar loading code and format backward compatibility

da924f5

csoler merged commit 7cfe231 into RetroShare:master Feb 8, 2026
1 check failed

jolavillette deleted the AvatarStorageAndUpdateImprove branch February 8, 2026 22:39

[WIP] Avatar persistence and synchronization #248

[WIP] Avatar persistence and synchronization #248

Uh oh!

Conversation

jolavillette commented Jan 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csoler commented Jan 24, 2026

Uh oh!

jolavillette commented Jan 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jolavillette commented Jan 27, 2026

Uh oh!

jolavillette commented Jan 28, 2026

Uh oh!

jolavillette commented Jan 28, 2026

Fix Avatar Persistence Regression

Uh oh!

csoler left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jolavillette commented Feb 7, 2026

Description

Changes

Verification

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants