fix: improve media upload handling and reduce memory pressure #2128
Conversation
|
Thanks for opening this pull request and contributing to the project! The next step is for the maintainers to review your changes. If everything looks good, it will be approved and merged into the main branch. In the meantime, anyone in the community is encouraged to test this pull request and provide feedback. ✅ How to confirm it worksIf you’ve tested this PR, please comment below with: This helps us speed up the review and merge process. 📦 To test this PR locally:If you encounter any issues or have feedback, feel free to comment as well. |
| logger | ||
| ) | ||
|
|
||
| if (result?.url || result?.directPath) { |
There was a problem hiding this comment.
directPath instead direct_path maybe was a mispelling of using any type
| }) | ||
| } | ||
|
|
||
| const uploadWithFetch = async ({ |
There was a problem hiding this comment.
@purpshell I created these two approaches because this is a bug in Node.js, so when it's fixed, we can remove the http/https approach. What do you think?
There was a problem hiding this comment.
media uploads should be on https though 1000% of the time, maybe throw an error if not? Weird to be uploading media to a E2EE service over http :V
There was a problem hiding this comment.
@purpshell this is to be used in tests hahaha (We up a http server for this). But I can remove if is really a problem
|
Why do we cache downloads that take up so much memory for no real benefit? It would be better to just download them again rather than bringing the server down every few minutes. |
I don't get it. This PR is related to upload only |
|
I know I’m not talking about this PR I’m talking about this. if (cacheableKey) {
const mediaBuff = await options.mediaCache.get(cacheableKey);
if (mediaBuff) {
logger?.debug({ cacheableKey }, 'got media cache hit');
const obj = proto.Message.decode(mediaBuff);
const key = `${mediaType}Message`;
Object.assign(obj[key], { ...uploadData, media: undefined });
return obj;
}
} |
|
@YonkoSam This is not enabled by default (you need to explicit enable in sock config) and we are not storing the download content data (bytes of the image or video), only the protobuf metadata things like mediaKey, url, etc. |
|
@jlucaso1 okey so the memory leak when uploading media comes from a different place |
|
Thanks for the update! I tested the changes on the However, I noticed a new regression with this fix: Image and Audio messages are failing to send. It seems only large document uploads are working fine, but standard media messages (images/audio) are not going through. Is there something specific regarding the media upload logic that was changed for these types? |
|
@AmilaPrabathKumara I need to check this. Thanks |
Okay, thanks. Let me know if you need any logs or further testing from my side. |
|
@AmilaPrabathKumara, could you try again? Actually, that was a regression, but it's fixed now (I added a test case to ensure it doesn't happen again). Thank you for helping with the testing. |
🤩 Thank you for the update! I just finished testing, and I can confirm the fix works perfectly. I tested with Images, Videos, Audio/PTT, and Documents — everything sends correctly now. Large file handling is also back to normal with stable memory usage. Since this resolves the regression completely, I hope this can be merged into master soon so everyone can benefit from the fix. Great work! 🚀 Thanks ❤️ @jlucaso1 |
|
Any updates or an estimated date to release this in RC10? |
It got merged into master now, RC10 is in a few days, so please test till then and report any bugs before we release the version |
closes: #2104
Tested with direct file url, direct file path, node and bun.