Skip to content

Conversation

siddarthkay
Copy link

@siddarthkay siddarthkay commented May 12, 2025

Thank you for the amazing work on this library!

Summary

I observed this issue in one of our hosts that run rust-synapse-compress-state

[2025-05-12T00:11:01Z ERROR panic] thread 'main' panicked at 'called `Result::unwrap()` on an `Err` 
value: Ran the compressor on a room that had no more work to do!': synapse_auto_compressor/src/main.rs:155
synapse-auto-compressor.service: Main process exited, code=exited, status=101/n/a

By looking at the code I observed that run_compressor_on_room_chunk is returning None for one of the rooms, indicating there's no more work to do in that room and I think it would be better if instead of bailing we could skip to the next room.

What do you think?

@siddarthkay siddarthkay requested a review from a team as a code owner May 12, 2025 05:06
Comment on lines +186 to +188
// Room is already fully compressed, skip to next room
debug!("Room {} is already fully compressed, moving to next room", room_to_compress);
continue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you tested this in real life?

It looks like this risks hot-looping on the same room again and again — after all, we just selected a new room to compress (therefore the query for finding compressible rooms believed it was compressible), we didn't modify the room in any way and now we're going to select a room to compress... what stops us from getting the same room again?

Bear in mind I'm not familiar with this codebase to be fair — but this seems to be my impression of the intention of the code — this bail is indicating a bug that we are selecting a room believing it is compressible but actually it isnt..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reivilibre I agree with your reading of the code. Getting into this branch indicates a deeper bug in the compressor, and we should bail instead of continuing on. get_next_room_to_compress relies on run_compressor_on_room_chunk calling write_room_compressor_state to update the state_compressor_progress table (here) in order to advance to the next room. If we continue here, that won't happen.

@siddarthkay are you still able to reproduce the issue? Perhaps we can work out why the compressor is selecting a room it can't compress.

@reivilibre reivilibre requested a review from a team May 16, 2025 13:25
Copy link
Member

@anoadragon453 anoadragon453 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants